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.

Sign up for GitHub

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

Jump to bottom

Throw an error if the current Julia version (Base.VERSION) is incompatible with the [compat] entry for julia in the Project.toml file #3526

Merged
merged 19 commits into from
Aug 25, 2023
Merged

Throw an error if the current Julia version (Base.VERSION) is incompatible with the [compat] entry for julia in the Project.toml file #3526

merged 19 commits into from
Aug 25, 2023

Conversation

Krastanov
Copy link
Contributor

@Krastanov Krastanov commented Jun 25, 2023 •
edited by DilumAluthge
Loading

Pkg now raises an error if the [compat] entry for julia is not met for a package.

This change was made due to new developers getting confused why their julia 1.8 does not work with a dev-ed package that has a julia compat set to 1.9.

There is no escape hatch, i.e. this behavior can not be disabled.

@Krastanov
do not silently accept broken julia compat
544281f 
Pkg now raises an error if the julia compat is not met for a package. `ENV["JULIA_PKG_CHECK_JULIA_COMPAT"]` can be used to override this check.

This change was made due to new developers getting confused why their julia 1.8 does not work with a dev-ed package that has a julia compat set to 1.9.
@Krastanov
Copy link
Contributor Author

Base.get_bool_env is not present in julia 1.9... I assume that is fine given that Base.get_bool_env is used elsewhere in Pkg.

@DilumAluthge
Copy link
Member

Even if the user has set JULIA_PKG_CHECK_JULIA_COMPAT, we should still print a warning, in my opinion.

@Krastanov
Copy link
Contributor Author

The TODO that I deleted used to say "either raise an error or be completely silent", so I did not want to overstep and followed those guidelines. Happy to change it though. What would be the best way for me to ask all stakeholders about their opinion? Should I tag someone here?

@Krastanov
Copy link
Contributor Author

Pinging @KristofferC as he is the one who wrote the "error or be quiet" TODO in #2179

@Krastanov
Copy link
Contributor Author

@KristofferC , @fredrikekre , just a friendly bump on this relatively minor change. Could you give me a rough idea whether this is something to be pursued and polished for merging or to be abandoned if it is contrary to how Pkg is supposed to work?

@Krastanov
Copy link
Contributor Author

bump?

@DilumAluthge
Copy link
Member

Bump @KristofferC @fredrikekre

IanButterworth
IanButterworth requested changes Aug 9, 2023
View reviewed changes
Copy link
Member

@IanButterworth IanButterworth left a comment

Choose a reason for hiding this comment

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

This seems like a good idea. It needs a test though. Perhaps create a test package that has an upper bound on an old julia version?

@IanButterworth
Copy link
Member

Nice. The authors of these test packages have me laughing 😂

@IanButterworth
Copy link
Member

Great. Can you also add a note to the NEWS.md

@DilumAluthge
Copy link
Member

Can we bikeshed the name of the environment variable?

@IanButterworth
Copy link
Member

Maybe it should be the reverse?

JULIA_PKG_JULIA_COMPAT_IGNORE=1 defaulting to off.

@Krastanov
Copy link
Contributor Author

I will wait a day before making changes to the variable name and add NEWS.md entry. Let me know if there are other suggestions.

@Krastanov
Copy link
Contributor Author

uhm... where is the NEWS.md? Maybe I am a bit sleep deprived, but I can not seem to find it

@Krastanov
Copy link
Contributor Author

I see a changelog file, but should I add this as a 1.10 or 1.11 entry?

@IanButterworth
Copy link
Member

Changelog, my bad. And 1.11

bkamins
bkamins reviewed Aug 9, 2023
View reviewed changes
src/Operations.jl Outdated
if julia_compat !== nothing && !(VERSION in julia_compat)
println(io, "julia version requirement for package $(err_rep(pkg)) not satisfied")
if Base.get_bool_env("JULIA_PKG_CHECK_JULIA_COMPAT", true) && !isnothing(julia_compat) && !(VERSION in julia_compat)
pkgerror("julia version requirement for package $(err_rep(pkg)) not satisfied; you can override and ignore this check by setting `ENV[\"JULIA_PKG_CHECK_JULIA_COMPAT\"]=0`")
Copy link
Member

@bkamins bkamins Aug 9, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Is this new check only enabled for packages or also projects shipped with Project.toml/Manifest.toml?

Also should we have =0 or =false in the error message?

Copy link
Member

Choose a reason for hiding this comment

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

I was under the impression that this is only for the active project's Project.toml, but I might be mistaken.

@IanButterworth Can you answer this?

@IanButterworth
Copy link
Member

To keep track of this I'm assigning @DilumAluthge here to help finish this up, given he raised discussion around some of this on slack

@Krastanov
Copy link
Contributor Author

I completely removed the env variable as suggested on slack. Happy to undo that if someone enforces a different preference

@DilumAluthge DilumAluthge changed the title By default, throw an error if the current Julia version (Base.VERSION) is incompatible with the [compat] entry for julia in the Project.toml file Throw an error if the current Julia version (Base.VERSION) is incompatible with the [compat] entry for julia in the Project.toml file Aug 15, 2023
DilumAluthge
DilumAluthge reviewed Aug 15, 2023
View reviewed changes
CHANGELOG.md Outdated Show resolved Hide resolved
@Krastanov @DilumAluthge
Update CHANGELOG.md
7f1ce98 
Co-authored-by: Dilum Aluthge <dilum@aluthge.com>
@DilumAluthge DilumAluthge self-requested a review August 15, 2023 12:46
@Krastanov
Copy link
Contributor Author

I think all change requests are fulfilled. Let me know if anything is missing.

IanButterworth
IanButterworth reviewed Aug 17, 2023
View reviewed changes
CHANGELOG.md Outdated Show resolved Hide resolved
@Krastanov @IanButterworth
Update CHANGELOG.md
24d815a 
Co-authored-by: Ian Butterworth <i.r.butterworth@gmail.com>
@DilumAluthge
Copy link
Member

I think this is good to go, but I'll do one last review this weekend to make sure.

@DilumAluthge DilumAluthge requested review from DilumAluthge and removed request for DilumAluthge August 17, 2023 05:35
@Krastanov
Copy link
Contributor Author

bump

DilumAluthge
DilumAluthge approved these changes Aug 21, 2023
View reviewed changes
Copy link
Member

@DilumAluthge DilumAluthge left a comment

Choose a reason for hiding this comment

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

One small suggested change, but otherwise LGTM.

test/api.jl Outdated Show resolved Hide resolved
@Krastanov @DilumAluthge
Update test/api.jl
689ff57 
Co-authored-by: Dilum Aluthge <dilum@aluthge.com>
@DilumAluthge
Copy link
Member

@IanButterworth I'll leave it to you to do the final review, and to click the merge button.

IanButterworth
IanButterworth approved these changes Aug 25, 2023
View reviewed changes
Copy link
Member

@IanButterworth IanButterworth left a comment

Choose a reason for hiding this comment

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

This has been discussed on slack and been open here for a while and I haven't seen any disagreement, so seems worth giving a go for 1.11. Thanks @Krastanov

@IanButterworth IanButterworth merged commit ff833e9 into JuliaLang:master Aug 25, 2023
13 checks passed
@DilumAluthge
Copy link
Member

Thanks @Krastanov!

@DilumAluthge
Copy link
Member

And thanks @IanButterworth for sheparding this thing through!

@Krastanov Krastanov deleted the patch-1 branch August 25, 2023 01:25
@IanButterworth IanButterworth mentioned this pull request Aug 28, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bkamins bkamins bkamins left review comments

@IanButterworth IanButterworth IanButterworth approved these changes

@DilumAluthge DilumAluthge DilumAluthge approved these changes

@KristofferC KristofferC Awaiting requested review from KristofferC

@fredrikekre fredrikekre Awaiting requested review from fredrikekre

Assignees

@IanButterworth IanButterworth

Labels
feature needs changelog
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Krastanov @DilumAluthge @IanButterworth @bkamins