-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
Comments
Just make a PR to fix the package? It is like a one line change? |
I think the best solution is to have |
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 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. |
@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 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). |
I think it only actually works on Linux. |
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.
Post the Julia version as well. |
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.
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.
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 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. |
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. |
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.
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. |
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 |
relevant PR #33777 |
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. |
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.
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
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. |
Does BinaryProvider |
Does it? This to me looks like it is only looking in |
Ah, that's because the latest release is actually on a different branch, not on |
Question: Should we include the |
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:
More asking out of curiosity, I assume that at this point we don't want to change things again. |
Lots of literature in #33777 |
Since 1.3 is done now, I'm going to say these questions are settled now, de facto |
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
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.
* 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>
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
and7z.dll
to thelibexec
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 theSys.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 really7z.exe
and7z.dll
need to stay in theSys.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
The text was updated successfully, but these errors were encountered: