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

Add news for LIBEXECDIR addition #33862

Closed
wants to merge 1 commit into from
Closed

Conversation

musm
Copy link
Contributor

@musm musm commented Nov 15, 2019

On the release-1.3 branch

@ararslan
Copy link
Member

It seems this isn't already in the NEWS file on master. The PR should be made against master and cherry picked onto the release branch.

@musm
Copy link
Contributor Author

musm commented Nov 16, 2019

Hi Alex, that's right, this is because the relevant PR was backported to 1.3 for the RC release. Thus it should just be present on the 1.3 news items.

@ararslan
Copy link
Member

Yes it should. My point is that the process here is backwards; it should be added to NEWS.md on master then go through the usual backporting process to apply to other releases.

@KristofferC
Copy link
Sponsor Member

KristofferC commented Nov 16, 2019

So the internal 7z now managed to sneak out to be documented?

@musm
Copy link
Contributor Author

musm commented Nov 16, 2019

@KristofferC mind clarifying what you mean?

@KristofferC
Copy link
Sponsor Member

Since when was providing 7z a part of the Julia programming language?

@ararslan
Copy link
Member

Mentioning the addition of the constant is fine, but we probably don't need to say anything about 7z.

@musm
Copy link
Contributor Author

musm commented Nov 16, 2019

I'm a bit confused on what you mean. We've always shipped with 7z, and yeah it's not required for the Julia language. It's path was changed from bin to libexecdir when staticfloat updated the build process so that 7z is provided through BinaryBuilder during the RC phase. The issue is that we still have some older packages relying on 7z being in the folder, and the primary purpose of this constant is to help those users transition to a more robust variable. Eventually the plan is to get rid of 7z, but I'm fine with not documenting the change. (I thought it would be worth to mention)

@KristofferC
Copy link
Sponsor Member

KristofferC commented Nov 16, 2019

The constant is internal as well. Just because an internally used executable (that some packages managed to find) is moved from one file to another doesn't mean that this is suddenly part of the stable api. This has already been pointed out. This shouldn't in my opinion not be documented or mentioned anywhere because the move of 7z was just a small internal file structure change. Relying on it means your code can break in future versions with (if you are lucky) an issue opened on the package.

@musm
Copy link
Contributor Author

musm commented Nov 16, 2019

Fair enough. Is appropriate to add a Compat for Base.LIBEXECDIR for older releases?

@musm musm closed this Nov 16, 2019
@KristofferC
Copy link
Sponsor Member

The issue is that we still have some older packages relying on 7z being in the folder,

Yeah, those packages have to fix that.

tkoolen added a commit to tkoolen/Blink.jl that referenced this pull request Dec 28, 2019
Really, we shouldn't be using 7z.exe at all since it's an internal
Julia utility (see e.g. JuliaLang/julia#33687
and
JuliaLang/julia#33862 (comment)).

In Julia 1.3+, we should probably switch to the Artifacts system at some
point, as was also done in
davidanthoff/Electron.jl#49 for Electron.jl. But
right now, we need a fix for Julia 1.0.5 as well.

This rather complex logic was introduced in JuliaGizmos#177. If 7z.exe is not in `joinpath(Sys.BINDIR, Base.LIBEXECDIR` (or its equivalent on versions prior to 1.3). Note that e.g. BinDeps, which is widely used, doesn't even go to the trouble of
tkoolen added a commit to tkoolen/Blink.jl that referenced this pull request Dec 28, 2019
JuliaGizmos#177 (comment)

This commit is really just to simplify behavior and avoid another wild goose
chase in the future. It does not address recent failures on 1.0.5, which
are just because the location of the 7z.exe that's shipped with Julia
binary distributions on Windows has also changed from 1.0.4 to 1.0.5.

Really, we shouldn't be using 7z.exe at all since it's an internal Julia utility (see e.g. JuliaLang/julia#33687 and JuliaLang/julia#33862 (comment)).

This rather complex logic was introduced in
JuliaGizmos#177 for the case that 7z.exe is not in `joinpath(Sys.BINDIR, Base.LIBEXECDIR` (or its equivalent on versions prior to 1.3). Note that e.g. BinDeps, which is widely used, doesn't even go to the trouble of doing this (https://github.com/JuliaPackaging/BinDeps.jl/blob/3a871c35ef1b0f45760f48088cabd7915838b0e3/src/BinDeps.jl#L116-L120).

7z.exe should only be absent on Windows in case of a native build; it's
copied over into the build tree in the `win-extras` `make` target, which is
not a dependency of the default `make` target
(see https://github.com/JuliaLang/julia/blob/d562715a8f78627627fd0ccf3197de670efbf810/doc/build/distributing.md#windows).

The previous logic attempted to find 7z.exe in likely install locations
of binary downloads of Julia. I'm personally not convinced by the
arguments given here:
JuliaGizmos#177 (comment).

In Julia 1.3+, we should probably switch to the Artifacts system at some point, as was also done in davidanthoff/Electron.jl#49 for Electron.jl. But right now, we need a fix for Julia 1.0.5 as well.
@musm musm deleted the patch-8 branch August 27, 2020 19:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants