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

Always allow stdlibs to update during a resolve #2787

Conversation

IanButterworth
Copy link
Sponsor Member

@IanButterworth IanButterworth commented Oct 21, 2021

@staticfloat I believe this might be a bug, and should be nothing by default?

See discussion in JuliaRegistries/General#47158

@staticfloat
Copy link
Sponsor Member

staticfloat commented Oct 21, 2021

Hmmm, setting julia_version to nothing has a larger effect than you're probably thinking. Setting it to nothing causes Julia version bounds to be completely ignored; e.g. you can instantiate a manifest that has a Julia v1.5- compatible package, and a Julia v1.6+ compatible package at the same time.

This is useful only in certain circumstances, such as wanting to get certain versions of a bunch of JLLs, regardless of their Julia compatibilities, so that you can build a new JLL linking against them.

@IanButterworth
Copy link
Sponsor Member Author

I see.

So keeping the historial stdlibs up to date is critical? And seemingly there's a gap in tests?

@staticfloat
Copy link
Sponsor Member

It seems silly to use the historical stdlibs for the current Julia version. Perhaps what is missing is a special-case in get_last_stdlibs() that checks if julia_version == VERSION, and uses stdlibs() (which loads the stdlibs from the current stdlib folder) instead of searching through STDLIBS_BY_VERSION.

@IanButterworth IanButterworth force-pushed the ib/fix_historicalstdlibs_skipping branch from 5ea2adc to 0893f06 Compare October 22, 2021 03:02
src/Operations.jl Outdated Show resolved Hide resolved
@IanButterworth
Copy link
Sponsor Member Author

Perhaps what is missing is a special-case in get_last_stdlibs() that checks if julia_version == VERSION, and uses stdlibs() (which loads the stdlibs from the current stdlib folder) instead of searching through STDLIBS_BY_VERSION.

Now implemented

@IanButterworth IanButterworth force-pushed the ib/fix_historicalstdlibs_skipping branch from dfb5e02 to e67cf9d Compare October 24, 2021 02:35
Comment on lines 341 to 347
jv = env.manifest.julia_version
manifest_same_version = !isnothing(jv) && jv.major == VERSION.major && jv.minor == VERSION.minor
# if the manifest was previously resolved by the same major-minor julia version, don't require stdlib versions
# to remain the same. This allows manifests that were generated on nightly julia versions, where stdlibs may change version
# to be updated

reqs = Resolve.Requires(pkg.uuid => (manifest_same_version && is_stdlib(pkg.uuid)) ? VersionSpec("*") : VersionSpec(pkg.version) for pkg in pkgs)
Copy link
Sponsor Member Author

@IanButterworth IanButterworth Oct 24, 2021

Choose a reason for hiding this comment

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

@staticfloat @KristofferC
I think this is the key to avoiding what's happening in JuliaRegistries/General#47158

My take is that the checked-in manifest that was resolved on a previous nightly had older stdlib versions, so here to make things a bit smoother for people checking in manifests generated on nightlies, we allow the stdlib versions to be updated.

i.e. before, I hit

(@v1.8) pkg> resolve
ERROR: Unsatisfiable requirements detected for package OpenBLAS_jll [4536629a]:
 OpenBLAS_jll [4536629a] log:
 ├─possible versions are: 0.3.17 or uninstalled
 └─restricted to versions 0.3.13 by an explicit requirement — no versions left

now I hit

(@v1.8) pkg> resolve
  No Changes to `~/.julia/environments/v1.8/Project.toml`
    Updating `~/.julia/environments/v1.8/Manifest.toml`
  [ea8e919c] ~ SHA ⇒ v0.7.0
  [4536629a] ↑ OpenBLAS_jll v0.3.13+7 ⇒ v0.3.17+2

@DilumAluthge
Copy link
Member

Just to be safe, instead of checking that the major and minor versions match, can we check that the major, minor, and patch versions match?

@IanButterworth IanButterworth force-pushed the ib/fix_historicalstdlibs_skipping branch from e67cf9d to a6530ae Compare October 24, 2021 02:45
@IanButterworth
Copy link
Sponsor Member Author

I think we guarantee that the stdlibs won't change version between patch versions? I'm trying to think through what people would expect. People that check in a manifest would likely expect the manifest to just work across all patches of a given major-minor julia version? That seems to be how General handles it? Anything more granular than that would cause similar headaches each patch release

reqs = Resolve.Requires(pkg.uuid => VersionSpec(pkg.version) for pkg in pkgs)

jv = env.manifest.julia_version
manifest_same_version = !isnothing(jv) && jv.major == VERSION.major && jv.minor == VERSION.minor
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
manifest_same_version = !isnothing(jv) && jv.major == VERSION.major && jv.minor == VERSION.minor
manifest_same_version = !isnothing(jv) && jv.major == VERSION.major && jv.minor == VERSION.minor && jv.patch == VERSION.patch

Copy link
Sponsor Member Author

Choose a reason for hiding this comment

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

See comment above

@DilumAluthge
Copy link
Member

I think we guarantee that the stdlibs won't change version between patch versions?

Do we? I know we try to guarantee it, but is that an absolute statement? E.g. in theory, if we need to apply a bugfix as part of a patch release, isn't it possible that we might need to update the stdlib version as part of that bugfix.

@DilumAluthge
Copy link
Member

I agree that we should try as hard as possible to avoid changing stdlibs in a patch release. But just in case we have to do it one day in the future in order to fix a serious bug, I think the code in this pull request should be future-proofed.

@IanButterworth
Copy link
Sponsor Member Author

Yeah, actually.. I'm not sure why I was thinking that would be the case. After all a patch is a patch, no reason why a stdlib wouldn't patch bump during a julia patch bump

@IanButterworth
Copy link
Sponsor Member Author

But I think, unless this whole idea is flawed (it could be) the right thing to do is to allow stdlibs to be updated across patch releases and downwards, so adding the patch check as you suggest above make that logic too specific

@IanButterworth IanButterworth changed the title Skip HistoricalStdlibs resolver tooling by default Allow stdlibs to update within same major-minor manifests. Skip HistoricalStdlibs resolver tooling by default. Update historical stdlibs. Oct 24, 2021
@IanButterworth IanButterworth force-pushed the ib/fix_historicalstdlibs_skipping branch 4 times, most recently from 7af346c to 4bf88fb Compare October 24, 2021 04:37
@DilumAluthge
Copy link
Member

DilumAluthge commented Oct 25, 2021

Well, in those cases, won't we just make sure to manually run the script to regenerate the list before we make a release of Julia?

If I understand correctly, the only use case we need to fix here is for people running on a nightly. So all we need to do is:

  1. Check that VERSION is a prerelease version.
  2. Check that the major, minor, and patch components of VERSION match the corresponding components of the Julia version number in the manifest.
  3. If both of the above conditions are correct, then instead of using the historical stdlibs list, we just look in the stdlib/ folder and get the version numbers from there.

If either condition 1 or condition 2 is false, then we should just use the historical stdlib list.

@IanButterworth
Copy link
Sponsor Member Author

If I understand correctly, the only use case we need to fix here is for people running on a nightly

Also anyone that would expect a given manifest to work across patch versions.

IMO versioned stdlibs should be allowed to be updated for a given manifest across julia patch versions, because:

  1. The user will be warned that the manifest came from a different julia version
  2. Julia cannot serve any other stdlib version than the one it's locked to
  3. Unversioned stdlibs like Pkg, Downloads etc are already free to float with the Julia version

@staticfloat
Copy link
Sponsor Member

IMO versioned stdlibs should be allowed to be updated for a given manifest across julia patch versions, because:

  1. The user will be warned that the manifest came from a different julia version
  2. Julia cannot serve any other stdlib version than the one it's locked to
  3. Unversioned stdlibs like Pkg, Downloads etc are already free to float with the Julia version

I feel this logic holds for Julia minor versions as well, though? Why not allow this for all Julia versions then?

@IanButterworth
Copy link
Sponsor Member Author

Yeah, you're right..

@IanButterworth IanButterworth force-pushed the ib/fix_historicalstdlibs_skipping branch from 1bd9eec to 35836d1 Compare October 25, 2021 20:50
@IanButterworth IanButterworth changed the title Allow stdlibs to update within same major-minor manifests. Skip HistoricalStdlibs resolver tooling by default. Update historical stdlibs. Always allow stdlibs to update. Skip HistoricalStdlibs resolver tooling by default. Update historical stdlibs. Oct 25, 2021
@tkf
Copy link
Member

tkf commented Oct 25, 2021

Does this PR solve the following MWE?

julia> VERSION
v"1.8.0-DEV.819"

(@v1.8) pkg> activate --temp
  Activating new project at `/tmp/jl_Aw4w8w`

(jl_Aw4w8w) pkg> add LinearAlgebra
...

(jl_Aw4w8w) pkg> resolve
ERROR: Unsatisfiable requirements detected for package OpenBLAS_jll [4536629a]:
 OpenBLAS_jll [4536629a] log:
 ├─possible versions are: 0.3.17 or uninstalled
 └─restricted to versions 0.3.13 by an explicit requirement — no versions left

Also, I wonder if this is a good smoke test to add? i.e., add all stdlib in an empty project and then try some operations like resolve and instantiate?

@DilumAluthge
Copy link
Member

I think maybe I don't understand what this PR is doing.

If we always allow stdlibs to update, then what is the purpose of the historical stdlibs list?

I thought the idea was that the historical stdlibs list exactly represents what stdlibs you would get with a given Julia version. It seems that with this PR, that guarantee is lost.

@DilumAluthge
Copy link
Member

DilumAluthge commented Oct 25, 2021

I was under the impression that we should always be using the historical stdlibs list, except in the single edge case in which VERSION is a nightly and the manifest Julia version is the same nightly, in which case we just use the versions from the stdlib/ folder.

Otherwise, the versions from the historical stdlibs list won't match what you would actually get in production... so what's the point of the historical stdlib list?

Probably it's the case that I don't understand this PR, or don't understand the historical stdlibs list, or both 😂

@IanButterworth
Copy link
Sponsor Member Author

I thought the idea was that the historical stdlibs list exactly represents what stdlibs you would get with a given Julia version. It seems that with this PR, that guarantee is lost.

The historical stdlibs tooling is only really used by BB. Otherwise, there's no need to touch it.
Pkg master currently does however use the historical list for normal use for the current version, but this PR adds a shortcut for given what @staticfloat said in #2787 (comment) which will make nightlies much more robust to stdlib version changes.

Does this PR solve the following MWE?

@tkf yes because of the shortcut mentioned above. (It also updates the historical stdlibs for future BB tooling usage)

@IanButterworth
Copy link
Sponsor Member Author

I wonder if this is a good smoke test to add?

Yeah, added. Thanks @tkf

@IanButterworth IanButterworth force-pushed the ib/fix_historicalstdlibs_skipping branch from 657c315 to 33c3e20 Compare October 25, 2021 23:35
@IanButterworth IanButterworth mentioned this pull request Oct 26, 2021
@IanButterworth IanButterworth force-pushed the ib/fix_historicalstdlibs_skipping branch from 33c3e20 to 7163d1c Compare October 26, 2021 14:38
@IanButterworth IanButterworth changed the title Always allow stdlibs to update. Skip HistoricalStdlibs resolver tooling by default. Update historical stdlibs. Always allow stdlibs to update Oct 26, 2021
@IanButterworth
Copy link
Sponsor Member Author

I merged the core stdlib handling updates in #2795 and left the question of always allowing stdlibs to update during a resolve in this PR, for further discussion/review

Copy link
Sponsor 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.

@KristofferC what do you think?

@IanButterworth
Copy link
Sponsor Member Author

@KristofferC Here's a demonstration of why I think this is needed JuliaRegistries/General#47659 (comment)

@KristofferC
Copy link
Sponsor Member

I'm quite out of the loop when it comes to versioned stdlibs and compat with them, setting julia_version for usage with BinaryBuilder etc. @staticfloat implemented most of that so his review is more valuable than mine.

As a side note, it is a bit unfortunate how this special case for BinaryBuilder has kind of ballooned in scope with all the surrounding infrastructure that is needed around it. Large dictionaries that need to be kept in sync with all Julia versions and even nightly with automated workflows to update them, arguments that need to be passed along more or less the whole codebase etc etc. I wonder if this really was the best way to do this.

@IanButterworth
Copy link
Sponsor Member Author

IanButterworth commented Nov 2, 2021

Large dictionaries that need to be kept in sync with all Julia versions and even nightly with automated workflows to update them

Just a note, nightly/the current version is no longer dependent on the hardcoded dictionaries as of #2795 which bypasses all the historical stdlib stuff under normal use.
But there are bots to keep it updated. Perhaps those could be only retriggered on new tags to Julia base @DilumAluthge

@DilumAluthge
Copy link
Member

Right now the bot is triggered only manually, but yeah we could do something fancier like automatically run it when there's a new Julia tag.

Anyway, for now, to run the bot manually, go here: https://github.com/JuliaLang/Pkg.jl/actions/workflows/run_hsg.yml

# Unless using the unbounded or historical resolver, always allow stdlibs to update. Helps if the previous resolve
# happened on a different julia version / commit and the stdlib version in the manifest is not the current stdlib version
unbind_stdlibs = julia_version === VERSION
reqs = Resolve.Requires(pkg.uuid => is_stdlib(pkg.uuid) && unbind_stdlibs ? VersionSpec("*") : VersionSpec(pkg.version) for pkg in pkgs)
Copy link
Sponsor Member Author

Choose a reason for hiding this comment

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

@staticfloat it just occurred to me, should it be this instead? Maybe VersionSpec("*") would've allowed updating to latest always, beyond what's allowed for the given julia version?

Suggested change
reqs = Resolve.Requires(pkg.uuid => is_stdlib(pkg.uuid) && unbind_stdlibs ? VersionSpec("*") : VersionSpec(pkg.version) for pkg in pkgs)
reqs = Resolve.Requires(pkg.uuid => is_stdlib(pkg.uuid) && unbind_stdlibs ? VersionSpec(stdlibs()[uuid][2]) : VersionSpec(pkg.version) for pkg in pkgs)

Copy link
Sponsor Member

Choose a reason for hiding this comment

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

I think in practice it doesn't matter, because as long as the compat ranges that we declare here match what is already in the system image, the resolver should choose those versions, right? The issue isn't that we're trying to install a different version than exists, it's that the resolver isn't being allowed to use the versions that are already installed, I thought?

Copy link
Sponsor Member Author

Choose a reason for hiding this comment

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

Sounds reasonable to me. I'll merge

@IanButterworth IanButterworth changed the title Always allow stdlibs to update Always allow stdlibs to update during a resolve Nov 2, 2021
@IanButterworth IanButterworth merged commit 816d76e into JuliaLang:master Nov 2, 2021
@IanButterworth IanButterworth deleted the ib/fix_historicalstdlibs_skipping branch November 2, 2021 19:55
@DilumAluthge
Copy link
Member

@KristofferC @IanButterworth Can we backport this to Julia 1.6 and 1.7?

@IanButterworth IanButterworth added backport 1.6 Change should be backported to release-1.6 backport 1.7 Change should be backported to release-1.7 labels Feb 21, 2022
@fredrikekre
Copy link
Member

Doesn't backport cleanly to 1.6 (can make a PR to #3033 if this is still needed).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport 1.6 Change should be backported to release-1.6 backport 1.7 Change should be backported to release-1.7
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants