New manifest: Set `julia_version` during resolve only, and warn if instantiating a manifest that was resolved in a different or unknown julia version #2620

IanButterworth · 2021-06-12T22:21:35Z

The intention is to use the new manifest format julia_version to guide the user when the manifest is (potentially) incompatible with the julia version.

The julia_version field in the manifest is now only set during resolve
Adds a @warn (maxlog = 1 per manifest path) when loading the manifest (any action that uses Context() plus a direct instantiate of a manifest)

For old format manifests:

┌ Warning: The active manifest file is an older format with no julia version entry. Dependencies may have been resolved with a different julia version.
└ @ ~/Documents/GitHub/Pkg.jl/test/manifest/formats/v1.0/Manifest.toml:0

For new format manifests with the julia_version field:

┌ Warning: The active manifest file has dependencies that were resolved with a different julia version (1.7.0-DEV.1199). Unexpected behavior may occur.
└ @ ~/Documents/GitHub/Pkg.jl/test/manifest/formats/v2.0/Manifest.toml:0

Questions:
When should the warning also be shown? After activate?

What if julia is started up via --project. Should there be a warning in the julia repl?

On master/non-released versions the full version check could create a lot of noise. Should the version check just be for major.minor? Edit: Implemented

DilumAluthge · 2021-06-12T22:23:50Z

On master/non-released versions the full version check could create a lot of noise. Should the version check just be for major.minor?

That makes sense to me.

DilumAluthge · 2021-06-12T22:24:05Z

What if julia is started up via --project. Should there be a warning in the julia repl?

Yeah I think this makes sense.

DilumAluthge · 2021-06-12T22:24:40Z

When should the warning also be shown? After activate?

Doing the warning on Pkg.activate seems fine to me.

IanButterworth · 2021-06-12T23:16:56Z

Updated to just check major.minor

Doing the warning on Pkg.activate seems fine to me.

AFAICT Pkg.activate currently doesn't load the manifest, so to add the warning at that step that would need to happen. Context() might be fast enough to not be noticeable. Not sure.

(github gripe.. it would be nice to reply to your messages as threads..)

DilumAluthge · 2021-06-13T00:04:05Z

AFAICT Pkg.activate currently doesn't load the manifest, so to add the warning at that step that would need to happen. Context() might be fast enough to not be noticeable. Not sure.

Hmmm. What are our alternatives here? We could do nothing on Pkg.activate, and just wait for the user to call some other Pkg operation? (Pkg.add, Pkg.update, Pkg.status, etc.)

IanButterworth · 2021-06-13T00:41:31Z

If the weird problems are only happening during precompilation, then the warning could be added to Context() and just triggered whenever that first loads.

But I think problems also occur during package load and regular usage.. so I think it does need to happen immediately after Pkg.actívate and julia startup after --project.

Perhaps in this PR we could add it to Context() and try that out, and see if more is needed?

IanButterworth · 2021-06-13T15:41:58Z

If this goes ahead, I think it should be backported to both 1.7 and 1.6.
With 1.6 the warning and tests will need a little tweaking during the rebase though

IanButterworth · 2021-06-15T11:57:54Z

@KristofferC at a minimum I think we should get the first commit here into 1.6.2 and the first 1.7 beta to fix #2621

KristofferC · 2021-06-16T19:35:59Z

warn if instantiating a manifest that was resolved in a different or unknown julia version

This doesn't really make sense to me. Why would you warn when you are resolving? At that point you are already doing the right thing (resolving on the new Julia version), or?

IanButterworth · 2021-06-16T19:53:59Z

I'm not suggesting warning when you resolve. Instantiating doesn't re-resolve, right? That's the time you expect the install to be repeatable, so seems like a good time to warn

KristofferC · 2021-06-16T20:12:54Z

My bad, I totally misread that. Yes, doing it during instantiate makes sense. 👍

KristofferC · 2021-06-16T20:13:45Z

src/Types.jl

@@ -337,6 +337,8 @@ function EnvCache(env::Union{Nothing,String}=nothing)
    write_env_usage(manifest_file, "manifest_usage.toml")
    manifest = read_manifest(manifest_file)

+    Types.check_warn_manifest_julia_version_compat(manifest, manifest_file)


I don't think this one should be here. This will trigger even when doing "API" style use (like in Pluto).

I removed this.

Ok so this PR now only warns during instantiate. It's obviously better than before, but not sure how many manifest compat issues it will catch given instantiate isn't that common, I believe

KristofferC · 2021-06-16T21:03:49Z

I just want to say that we should be quite careful with adding warnings. Warning spam can be a bit annoying and a warning should (in my opinion) pretty much only be printed when there is a direct immediate action you should take as a response to fix the warning.

I'm getting a little bit worried with all the logic for these added warnings. Ideally, it should be kept to a minimum.

IanButterworth · 2021-06-16T21:07:07Z

I totally agree. I don't think I figured out a good design, but this PR now has the minimal example via instantiate so at least people can try that out

fredrikekre · 2021-07-01T07:07:34Z

Looks like this is already on release-1.7?

set julia_version in manifest during resolve only

52a2eeb

DilumAluthge requested a review from KristofferC June 12, 2021 22:23

IanButterworth force-pushed the ib/new_manifest_set_julia_version_during_resolve branch from 93a1d58 to a4135cc Compare June 12, 2021 23:27

IanButterworth force-pushed the ib/new_manifest_set_julia_version_during_resolve branch 3 times, most recently from db56ed8 to 7712db0 Compare June 13, 2021 15:39

IanButterworth mentioned this pull request Jun 14, 2021

Pkg never adds a julia_version entry to an upgraded manifest #2621

Closed

IanButterworth added backport pending 1.6 labels Jun 14, 2021

KristofferC reviewed Jun 16, 2021

View reviewed changes

KristofferC force-pushed the ib/new_manifest_set_julia_version_during_resolve branch from 7712db0 to b710f8c Compare June 16, 2021 20:21

IanButterworth added 3 commits June 16, 2021 22:43

warn when loading a manifest from a different julia version

6b301fc

add more info to manifest parsing error log

8afd1cb

use less common home dir test project name

91af68a

KristofferC force-pushed the ib/new_manifest_set_julia_version_during_resolve branch from b710f8c to 91af68a Compare June 16, 2021 20:44

KristofferC merged commit f08c9e2 into JuliaLang:master Jun 16, 2021

IanButterworth deleted the ib/new_manifest_set_julia_version_during_resolve branch June 16, 2021 21:06

fredrikekre removed the backport pending 1.7 label Jul 1, 2021

This was referenced Jul 7, 2021

no method matching joinpath(::Nothing) during download_source on 1.6.2-pre #2665

Open

[release-1.6] Add manifest julia_version difference warning #2666

Merged

IanButterworth mentioned this pull request Nov 11, 2021

Backports release 1.6 #2843

Merged

fredrikekre removed the backport pending 1.6 label Mar 18, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New manifest: Set `julia_version` during resolve only, and warn if instantiating a manifest that was resolved in a different or unknown julia version #2620

New manifest: Set `julia_version` during resolve only, and warn if instantiating a manifest that was resolved in a different or unknown julia version #2620

IanButterworth commented Jun 12, 2021 •

edited

Loading

DilumAluthge commented Jun 12, 2021

DilumAluthge commented Jun 12, 2021

DilumAluthge commented Jun 12, 2021

IanButterworth commented Jun 12, 2021

DilumAluthge commented Jun 13, 2021

IanButterworth commented Jun 13, 2021 •

edited

Loading

IanButterworth commented Jun 13, 2021

IanButterworth commented Jun 15, 2021

KristofferC commented Jun 16, 2021

IanButterworth commented Jun 16, 2021

KristofferC commented Jun 16, 2021

KristofferC Jun 16, 2021

KristofferC Jun 16, 2021

IanButterworth Jun 16, 2021

KristofferC commented Jun 16, 2021

IanButterworth commented Jun 16, 2021

fredrikekre commented Jul 1, 2021

New manifest: Set julia_version during resolve only, and warn if instantiating a manifest that was resolved in a different or unknown julia version #2620

New manifest: Set julia_version during resolve only, and warn if instantiating a manifest that was resolved in a different or unknown julia version #2620

Conversation

IanButterworth commented Jun 12, 2021 • edited Loading

DilumAluthge commented Jun 12, 2021

DilumAluthge commented Jun 12, 2021

DilumAluthge commented Jun 12, 2021

IanButterworth commented Jun 12, 2021

DilumAluthge commented Jun 13, 2021

IanButterworth commented Jun 13, 2021 • edited Loading

IanButterworth commented Jun 13, 2021

IanButterworth commented Jun 15, 2021

KristofferC commented Jun 16, 2021

IanButterworth commented Jun 16, 2021

KristofferC commented Jun 16, 2021

KristofferC Jun 16, 2021

Choose a reason for hiding this comment

KristofferC Jun 16, 2021

Choose a reason for hiding this comment

IanButterworth Jun 16, 2021

Choose a reason for hiding this comment

KristofferC commented Jun 16, 2021

IanButterworth commented Jun 16, 2021

fredrikekre commented Jul 1, 2021

New manifest: Set `julia_version` during resolve only, and warn if instantiating a manifest that was resolved in a different or unknown julia version #2620

New manifest: Set `julia_version` during resolve only, and warn if instantiating a manifest that was resolved in a different or unknown julia version #2620

IanButterworth commented Jun 12, 2021 •

edited

Loading

IanButterworth commented Jun 13, 2021 •

edited

Loading