Skip to content
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

Upgrade and use NSIS for compression instead of extra 7z stage #31879

Merged
merged 1 commit into from
May 2, 2019

Conversation

musm
Copy link
Contributor

@musm musm commented Apr 29, 2019

For a reference see the original issue #31875

Fix #31875
This also fixes #30475

@ViralBShah
Copy link
Member

Does this mean we can completely remove the dependency on 7z?

@ViralBShah ViralBShah added system:windows Affects only Windows building Build system, or building Julia or its dependencies labels Apr 29, 2019
@musm
Copy link
Contributor Author

musm commented Apr 29, 2019

I don't think so. Julia ships with 7z but I'm not exactly sure what it's used for.

@musm
Copy link
Contributor Author

musm commented Apr 30, 2019

I searched the repo and indeed 7z isn't used by any julia source files either in base or stdlib. We can work towards that goal in another PR since that is an unrelated change.

@ararslan ararslan requested a review from staticfloat April 30, 2019 17:43
@staticfloat
Copy link
Member

staticfloat commented Apr 30, 2019

Does this mean we can completely remove the dependency on 7z?

No; we use 7z to extract .tar.gz files on Windows. We use it in things like BinaryProvider and Pkg

@staticfloat
Copy link
Member

staticfloat commented Apr 30, 2019

Does this effect compression factor at all?

@musm
Copy link
Contributor Author

musm commented Apr 30, 2019

Does this affect compression factor at all?

No, the binaries are almost the same size (I can't remember the size difference anymore)

@staticfloat
Copy link
Member

Is there any discernable difference between the installation experience? E.g. for people who do headless installs, are they going to need to change the flags they use to do a headless install? If so, we may need to note that in NEWS.md.

@musm
Copy link
Contributor Author

musm commented Apr 30, 2019

The only differences is that it's a smoother installation experience from a user perspective. Instead of an extraction pop window and then the installation phase, the two are merged into the nsis installer and extraction and copying both occur during the installation phase.

E.g. for people who do headless installs, are they going to need to change the flags they use to do a headless install?

No change required. With the 7z stage it merged the extraction details into the binary. The only change that might be necessary are scripts that first extract the exe using 7z, since they know that the binary was first compressed. However this must be very rare since they would have to know how that the binary was first compressed using 7z.

The reason I delved into this was I got super annoyed at how chocolatey kept popping up the Julia extraction stage of the installer even though the silent flag of the nsis installer was used. The reason for this is that the 7z configuration is unaffected by the silent flag.

@musm
Copy link
Contributor Author

musm commented Apr 30, 2019

BTW I plan to also update and make the installer look more modern in another PR.

@musm
Copy link
Contributor Author

musm commented May 1, 2019

size difference:
This pr: 58929 KB
master: 52669 KB

I can increase the dictionary size if this difference is too big @staticfloat ?

@musm
Copy link
Contributor Author

musm commented May 1, 2019

size difference reduced to less than 2MB

Copy link
Member

@staticfloat staticfloat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@StefanKarpinski
Copy link
Member

I restarted the Linux 64 Travis build because it wasn't 100% clear to me that it was unrelated. Also unclear to me if @staticfloat's "LGTM" was about the code, the test failures or both.

@musm
Copy link
Contributor Author

musm commented May 2, 2019

Squashed the commits and cleaned things up. @staticfloat want to take another look before merge?
I've tested this out several times and things are working nicely. Binaries have a comparable size (within 2 MB) and build times are a bit faster for binary-dist.

@musm musm changed the title Update to latest NSIS and use NSIS for compression instead of extra 7z stage Upgrade and use NSIS for compression instead of extra 7z stage May 2, 2019
@staticfloat staticfloat merged commit b118e68 into JuliaLang:master May 2, 2019
@musm
Copy link
Contributor Author

musm commented May 2, 2019

Awesome thanks so much, I'm glad to have this improvement.

@musm musm deleted the nsis branch May 2, 2019 17:28
musm added a commit to musm/julia that referenced this pull request May 3, 2019
@musm musm restored the nsis branch May 6, 2019 16:24
@staticfloat
Copy link
Member

@musm I'm having trouble with win32 installers now, and I think it might be related to this. Previously, I've been using the following command to silently extract the Julia installer's files into the current directory for CI:

./julia*.exe /S /D=$(cygpath -w $(pwd))

But when I run that on a win32 build now, it doesn't put anything in the current directory. Could this be related to these changes? For reference, you can see this behavior in this file.

@musm
Copy link
Contributor Author

musm commented May 11, 2019

Unlikely, but I'll take a look Sunday at the earliest. Is that exe from this branch, since we reverted these changes anyways?

@staticfloat
Copy link
Member

Ah, you're right, I forgot that we reverted this. Hmmm, I wonder what could have changed then... Furthermore, it looks like it's only happening on win32, not on win64, which I think rules these changes out.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
building Build system, or building Julia or its dependencies system:windows Affects only Windows
Projects
None yet
4 participants