-
-
Notifications
You must be signed in to change notification settings - Fork 5.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Prepend a version number too in source tarballs #29738
Conversation
It is required. These targets should be able to be run on any system that can build Julia. |
Different approach now: as we are using an explicit file list, it actually shouldn't be a problem to just temporarily create a symlink in the source tree which points back to the root. I would hope that this would work properly on all UNIX-y platforms. Less certain about Windows though. |
I checked that this works as intended on Windows in a Cygwin build. And by interpolation, I hope that it works on everything in between. |
Makefile
Outdated
sed -e "s_.*_$$DIRNAME/&_" light-source-dist.tmp > light-source-dist.tmp1; \ | ||
cd ../ && tar -cz --no-recursion -T $$DIRNAME/light-source-dist.tmp1 -f $$DIRNAME/julia-$(JULIA_VERSION)_$(JULIA_COMMIT).tar.gz | ||
ln -s . $$DIRNAME || exit 1; \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why || exit 1
? I'm not sure I see why that would change any outcome?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I always assumed that just exit
would just return 0
, but it turns out that is not the case -- it returns $?
. Removed the 1
s.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
exit 1
explicitly causes bash
to exit with return code 1
, so I think what you want is exit 0
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, I'm not sure that exit 0
is what you want. What are you trying to do? Do you want to ignore ln -s
errors? Or do you want ln -s
errors to prevent running the tar
command? Or do you want ln -s
errors to run the tar
command but prevent the make
build from finishing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If ln
fails, I want this target to exit right away with an error. I'd say exit
or exit N
where N != 0
is what I want.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the best thing to do then is to not have an exit
at all, and replace ln -s . || exit 1; \
with ln -s . && \
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also need to connect tar
and rm
with &&
then. And could connect sed
too -- unlikely to fail, but might as well check.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I think that's a good idea. Best to check all exit codes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It’s a Makefile—it’s already checking the exit code of each bash process (unless prefixed with -). No need to duplicate that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, .|| exit
is the same as || true
, since the default exit code is success
Edit: reading above, if it propagates, then it’s equivalent to not being present
I'm actually quite surprised this worked on Windows Cygwin; are you on a recent Windows 10? I believe that anything older than a recent Windows 10 might require admin privileges to create symlinks. (I'm not that worried about it since this is an unusual operation, so as long as it works for "some" windows machines I'm okay with it) |
Instead of transforming the file names, just create a temporary symlink in the source directory that points back to the root of the source directory. As long as symlinks work properly, this should be a portable solution.
It should be latest -- it is a fresh install from a few days ago on a VM with all the post-install updates (says version 10.0.17134). But in any case, doesn't Cygwin implement symlinks as normal files from the Windows perspective? I am basing this on this SO answer, and when I tried it out and looked at the symlink in the Windows file manager, it indeed appears to be just a normal file. |
60a7f35
to
3fa7ec7
Compare
AV failure seems unrelated. |
I didn't realize this; that's great. |
As far as I could tell, the only reason why the commands were joined together into a single shell call with |
Friendly bump -- anything left to do here? |
Seems fine to me but I'm not sure I'm qualified to review. Who would you like to review? |
Not sure. Maybe @staticfloat or @vtjnash could take another look? |
Looks good to me. Thanks @mortenpi |
.....ah I clicked "rebase and merge", not "squash and merge". 🙄 |
Currently the files in source tarballs just have
julia/
prepended to them (e.g. 1.0.1 tarball), but I propose including the version number too, i.e. prependjulia-x.y.z/
.I switched over to using the
--transform
option oftar
for modifying the filenames. However, this might not be very portable -- it seems to be that--transform
/--xform
is specific to GNU tar. BSD tar does not appear to have it.. or rather calls a similar option-s
if I understand correctly.How important is it to keep these targets cross-platform? I do not have any other idea how to approach this at the moment, other than doing something similar to the original implementation of
cd
-ing up and then renaming or symlinking the directory, or copying files over to temporary directory.