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

Update packages that rely on 7z.exe being in the Sys.BINDIR folder #33687

Closed
davidanthoff opened this issue Oct 26, 2019 · 22 comments
Closed

Update packages that rely on 7z.exe being in the Sys.BINDIR folder #33687

davidanthoff opened this issue Oct 26, 2019 · 22 comments

Comments

@davidanthoff
Copy link
Contributor

davidanthoff commented Oct 26, 2019

EDIT: Please take a look at the end of this issue for what the TODO is at this point, after discussing this issue.

If I understand the recent changes to this, then the idea for julia 1.3 is to move 7z.exe and 7z.dll to the libexec folder, right? That is how I'm interpreting #33493.

But, I think that is a change that will break a whole bunch of packages that have an assumption built in that 7z.exe is actually located in the Sys.BINDIR folder. Examples are here, here and here (I believe there are more packages that have the same issue). I think those are way too central packages to break, so I think really 7z.exe and 7z.dll need to stay in the Sys.BINDIR folder, given that there is a promise that Julia 1.x versions are not breaking.

I believe this should be blocking for 1.3.

Did PkgEval pick this up? Or is PkgEval only run regularly on Linux? I think this is an example of why it would be crucial to run PkgEval on Windows as part of the regular release process.

CC @staticfloat

@KristofferC
Copy link
Sponsor Member

Just make a PR to fix the package? It is like a one line change?

@musm
Copy link
Contributor

musm commented Oct 26, 2019

I think the best solution is to have Sys.LIBEXECDIR ref #33493 (comment)

@davidanthoff
Copy link
Contributor Author

Just make a PR to fix the package? It is like a one line change?

That is not the problem. The problem is that there is a promise that old code will work on newer versions of Julia 1.x, and this breaks that promise. And as far as I can tell, this will be very widespread: pretty much anything that has a binary dependency is probably affected on Windows, right?

The particular scenario I have in mind is this: someone published a replication code for a paper. It is a zip file, hosted on something like zenodo.org, with a Project.toml and Manifest.toml in the root. The replication instructions say to to pkg> activate . and pkg> instantiate and then run a script. With this change, that won't work anymore. I think the backwards compat promise really needs to cover this scenario, at least for the academic setting this is really a core use-case.

In general, I really hope that the interpretation of "we don't break things" is not interpreted "if there is a simple one line fix that can be applied in user code for a breaking change in Julia, we are ok to include that in a Julia 1.x release". I feel strongly that backwards compat should work under the assumption that old code needs to run without any modifications. There might obviously be corner cases, and I think a good criterion in that case is to look at how large spread the impact of a change is. The impact of this particular change to me seems very large, because I think it will break pretty much anything that has a binary dependency on Windows.

@musm
Copy link
Contributor

musm commented Oct 26, 2019

@davidanthoff I understand where you're coming from, but the number of packages this affects is actually quite small, and is less a problem than Julia pre version 1, since we have now transitioned to BinaryProvider.

I actually made PRs to roughly 8 package a while ago when we had to explicitly specify 7z.exe due to some internal changes. (These were all the public packages I found that use 7z.exe.)

Looking back only 7 of these packages need to get updated to accommodate the latest changes (the others have transitioned to BinaryProvider).

(1) The best solution is to provide Sys.LIBEXECDIR and a suitable Compat definition.

Affected packages include Electron.jl Taro.jl TimeZones.jl BinaryProvider.jl MXReader.jl Weber.jl BinDeps.jl

I'm more than happy to make suitable PRs to these packages to accommodate these packages.

But for the PRs to be robust to future changes we should probably first do (1).

@ararslan
Copy link
Member

Or is PkgEval only run regularly on Linux?

I think it only actually works on Linux.

@KristofferC
Copy link
Sponsor Member

KristofferC commented Oct 26, 2019

The problem is that there is a promise that old code will work on newer versions of Julia 1.x, and this breaks that promise.

That has never been a promise (it is impossible to fulfill since literally any change is possible to observe and therefore relied on by a package). The promise is to not break public API.

The particular scenario I have in mind is this: someone published a replication code for a paper.

Post the Julia version as well.

@davidanthoff
Copy link
Contributor Author

the number of packages this affects is actually quite small, and is less a problem than Julia pre version 1, since we have now transitioned to BinaryProvider.

I don't think that is the right metric here. Any package that depends on BinaryProvider.jl, or BinDeps.jl is currently affected by this issue. That is a huge number of packages.

The problem is that there is a promise that old code will work on newer versions of Julia 1.x, and this breaks that promise.

That has never been a promise (it is impossible to fulfill since literally any change is possible to observe and therefore relied on by a package). The promise is to not break public API.

I understood @StefanKarpinski here as saying that essentially minor versions won't break things in the package ecosystem ("[...] which, in fact, do not break things in the package ecosystem as determined by running PkgEval to verify that there isn’t any breakage."). This change here clearly breaks a lot of things in the package system, so I think it should not be made. Now, I guess one can argue that this is an internal change, not part of the public API, and therefore ok. I'm not a fan of such an argument. To the users that are affect by this it doesn't matter whether this was ok by a narrow reading of some policy, at the end of the day things don't work for them and I think the goal should be to avoid that.

The particular scenario I have in mind is this: someone published a replication code for a paper.

Post the Julia version as well.

I don't think that is a good solution. I think it would be way more user friendly if newer 1.x versions simply run code that ran under a previous version of Julia. I'm pretty positive that most folks that read the official release plan process blog would expect that. Also, if this is a legit argument, then why do we have to care about backwards compat at all? We can always tell folks to just run things with an older version of Julia.

It seems to me that another factor in a decision like this is how bad a problem is fixed by this change. I agree that in principle this change is for the good, but on the other hand, are we aware of a single bug report where someone had a problem with the pre 1.3 approach? If not (or the number of users for whom this is a problem is low), I think that is one more argument to make this change in Julia 2.x.

I also found another bit of info re testing this: I think testing BinaryProvider.jl on the latest Windows 10 version won't unearth this problem: it turns out that MS started shipping tar as part of Windows 10 in system32 in more recent Windows 10 versions, so BinaryProvider.jl then just falls back to using that by default on Julia 1.3. But, I believe that both older versions of Windows 10, Windows versions pre 10, and julia 32 bit on Windows 10 don't take that path, and things fail. I think that explains why the 64 bit tests pass, but the 32 bit tests fail in the tests for SymbolServer.jl.

I think at the end of the day that changes that break existing package versions and affect a large number of packages (direct or indirect) shouldn't ship as a minor version.

@Keno
Copy link
Member

Keno commented Oct 27, 2019

There is no guarantee that code in 1.x will run as is in 1.x+1. The guarantee is that we won't change stable interfaces. The location or existence of 7z is not a stable interface. The packages that need to know about it will get fixed and users will be happy. For exact reproducibility the julia version always needs to be included. This is exactly the difference between something that can go into a patch release and something that can go into a minor release. I do agree getting the packages fixed should block the 1.3 release.

@KristofferC
Copy link
Sponsor Member

I don't think that is a good solution. I think it would be way more user friendly if newer 1.x versions simply run code that ran under a previous version of Julia.

Unless you vet all the dependencies to make sure they are not relying on internals of Julia you need to give the Julia version as well to ensure reproducibility.

then why do we have to care about backwards compat at all? We can always tell folks to just run things with an older version of Julia.

Because we want people that program against the public stable interface to have their code still work because that makes Julia a nicer language to use. But for things where you really want proper reproducibility you want to give Julia version + Manifest anyway.

@davidanthoff
Copy link
Contributor Author

Ok, I have to say, I'm not happy with this policy, but ok.

In any case, can someone add this issue to the Julia 1.3 milestone, where the actual TODO would be to update all the packages that currently rely on the old location for 7z. As @Keno suggested, I think 1.3 should only be released once those packages have new versions tagged that are updated.

@davidanthoff davidanthoff changed the title 7z.exe should be in Sys.BINDIR folder on Windows Update packages that rely on 7z.exe being in the Sys.BINDIR folder Nov 12, 2019
@musm
Copy link
Contributor

musm commented Nov 12, 2019

relevant PR #33777

@KristofferC
Copy link
Sponsor Member

I think 1.3 should only be released once those packages have new versions tagged that are updated.

We don't hold the release for a bunch of other packages that rely on internals (but we try to open issues when we find them).

This issue has been opened for soon 3 weeks. Since the fix is so small, it either isn't that important, or everyone wants someone else to do the job. Just cutting the 1.3 release should make someone step up.

@davidanthoff
Copy link
Contributor Author

bunch of other packages

Well, in this case that includes BinaryProvider.jl, which is a) very widely used and b) the official binary story from the core Julia team, as far as I can tell. Yes, you can tell a story that BinaryProvider.jl used non-public APIs and therefore it is ok to break it, but I think a much more user friendly approach is to just make sure it works before 1.3 ships.

it either isn't that important

I think it is important but not widely detected at this point. As far as I can tell, anything using BinaryProvider.jl will not work on any Windows version that is not of the latest kind (where tar ships out of the box). I don't have the latest numbers at hand, but if I remember our telemetry from the VS Code extension that is potentially a very large group of users. At the same time it seems also super likely that the crowd of folks that actually develop Julia is not running into this issue because a) all the CI Windows solutions run on the latest version of Windows (so the only breakages we see right now are on 32 bit Windows, where almost all builds for all my packages are broken because of this), b) devs might be on a recent Windows version, c) devs might have some of these unpacking programs in their PATH.

This issue has been opened for soon 3 weeks

Well, I would have fixed the problem in my packages, but we needed #33777 first and then a new RC build that includes that so that I can actually test it.

So, I think at least BinaryProvider.jl should really be in a working shape before 1.3 ships, and so I think that warrants adding this issue to the 1.3 milestone.

@staticfloat
Copy link
Sponsor Member

So, I think at least BinaryProvider.jl should really be in a working shape before 1.3 ships, and so I think that warrants adding this issue to the 1.3 milestone.

Does BinaryProvider v0.5.8 not work for you? It should be looking in both bin as well as libexec, as desired.

@davidanthoff
Copy link
Contributor Author

Does BinaryProvider v0.5.8 not work for you? It should be looking in both bin as well as libexec, as desired.

Does it? This to me looks like it is only looking in bin. But I can't really test it, because the last binary for Julia I have is RC4, and I think there the whole story is broken because the exe and the dll are in different directories, right?

@staticfloat
Copy link
Sponsor Member

Ah, that's because the latest release is actually on a different branch, not on master. This was the easiest way to roll back a bad release without outright reverting commits, but those commits will probably never be fixed now that 1.3 is trying to eradicate BinaryProvider.

@davidanthoff
Copy link
Contributor Author

Hehe, that was well hidden :) Great!

Is that the recommended current way of handling this in other packages as well? Or should I use #33777 instead?

Any chance we might get a new RC soonish that also includes #33777? That would make it a lot easier to fix this stuff in packages.

@musm
Copy link
Contributor

musm commented Nov 16, 2019

Question: Should we include the LIBEXECDIR addition to Compat? (see: JuliaLang/Compat.jl#676) I'm having second thoughts because as was pointed out this is not part of the stable API, but at the same time it can help existing users more easily upgrade their packages?

@giordano
Copy link
Contributor

@davidanthoff
Copy link
Contributor Author

Ok, so the right code to use is:

if isdefined(Base, :LIBEXECDIR)
  const exe7z = joinpath(Sys.BINDIR, Base.LIBEXECDIR, "7z.exe")
else
  const exe7z = joinpath(Sys.BINDIR, "7z.exe")
end

right? I'll do that for Electron.jl and NodeJS.jl.

Having said that, two questions:

  1. Why is LIBEXECDIR not in the Sys module like BINDIR?
  2. Why is LIBEXECDIR not an absolute path, like BINDIR? Seems weirdly asymmetric...

More asking out of curiosity, I assume that at this point we don't want to change things again.

@giordano
Copy link
Contributor

Having said that, two questions:

  1. Why is LIBEXECDIR not in the Sys module like BINDIR?
  2. Why is LIBEXECDIR not an absolute path, like BINDIR? Seems weirdly asymmetric...

More asking out of curiosity, I assume that at this point we don't want to change things again.

Lots of literature in #33777

@vtjnash
Copy link
Sponsor Member

vtjnash commented Nov 25, 2019

Since 1.3 is done now, I'm going to say these questions are settled now, de facto

@vtjnash vtjnash closed this as completed Nov 25, 2019
tkoolen added a commit to tkoolen/Blink.jl that referenced this issue Dec 28, 2019
tkoolen added a commit to tkoolen/Blink.jl that referenced this issue 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 issue 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.
twavv pushed a commit to JuliaGizmos/Blink.jl that referenced this issue Dec 28, 2019
* Fixed new location of 7z.exe in Julia 1.3.0

In Julia 1.3.0 7z.exe is moved into the /libexec folder.

* Checking for version 1.3.0

Now check for if Julia >= 1.3.0 and select the right location for 7.exe.

* Tweak 7z path creation.

Based on JuliaLang/julia#33687 (comment).

* Make indentation consistent, at least within install.jl.

* Just use BinDeps.unpack_cmd.

`unpack_cmd` is exported from BinDeps. Let them deal with the 7zip
stuff, we don't need to do anything special in this package.

Co-authored-by: pgunnink <38908229+pgunnink@users.noreply.github.com>
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

No branches or pull requests

8 participants