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

Handling extensions in Project.toml #105

Closed
gdalle opened this issue Apr 22, 2023 · 14 comments
Closed

Handling extensions in Project.toml #105

gdalle opened this issue Apr 22, 2023 · 14 comments

Comments

@gdalle
Copy link
Contributor

gdalle commented Apr 22, 2023

Julia 1.9 introduces weak dependencies and extensions, which are specified in Project.toml. During my tests, it seems Aqua.jl demands another order of sections than the other tools I use (Pkg.jl and PackageCompatUI.jl). Is that possible? If so, what would be the rationale?

Aqua order:

  • deps
  • compat
  • extensions
  • extras
  • targets
  • weakdeps

Pkg order:

  • deps
  • weakdeps
  • extensions
  • compat
  • extras
  • targets
gdalle added a commit to gdalle/Aqua.jl that referenced this issue Apr 24, 2023
DilumAluthge added a commit that referenced this issue May 17, 2023
* Fix project order for 1.9

Solves #105

* Apply suggestions from code review

---------

Co-authored-by: Dilum Aluthge <[email protected]>
@jishnub
Copy link
Contributor

jishnub commented May 17, 2023

It seems this is fixed by #106 and may be closed?

@gdalle
Copy link
Contributor Author

gdalle commented May 17, 2023

Yes indeed!

@gdalle gdalle closed this as completed May 17, 2023
@gdalle gdalle reopened this May 17, 2023
@gdalle
Copy link
Contributor Author

gdalle commented May 17, 2023

I think this fix might not work for all Julia versions: with exactly the same Project.toml, Aqua's latest checks pass on 1.9 but fail on 1.6 in https://github.com/gdalle/ImplicitDifferentiation.jl/actions/runs/5001070658/jobs/8959276447?pr=41
Probably due to Pkg.jl reordering stuff in different ways?

@gdalle
Copy link
Contributor Author

gdalle commented May 17, 2023

Aqua order:

deps
compat
extensions
extras
targets
weakdeps

I confirm that Pkg orders sections in this way for 1.6 but not 1.7, 1.8, 1.9

@gdalle
Copy link
Contributor Author

gdalle commented May 17, 2023

If anyone runs into this, my solution is to only test project formatting whenever the Julia version is > 1.6

@fingolfin
Copy link
Collaborator

The output of your CI log indeed indicates that something else modified the Project.toml contents and the result then differed from what is expected / is in the repository.

Indeed, the Run julia-actions/julia-buildpkg@v1 step says:

...
   Installed Compat ────────────────── v4.6.1
   Installed FastClosures ──────────── v0.3.2
    Updating `~/work/ImplicitDifferentiation.jl/ImplicitDifferentiation.jl/Project.toml`
  [c29ec348] + AbstractDifferentiation v0.5.2
  [d360d2e6] + ChainRulesCore v1.16.0
  [ba0b0d4f] + Krylov v0.9.1
  [5c8ed15e] + LinearOperators v2.5.2
  [ae029012] + Requires v1.3.0
...

The same for the Julia nightly test.

I don't understand why this happens right now, though... and in any case, it seems kinda problematic, because it also defeats the purpose of the whole test: if we let Pkg rewrite Project.toml before letting Aqua.jl inspect it, then it will always pass if Julia is new enough.

Does anyone know...

  • how I could reproduce this locally? I tried Pkg.test(Aqua) but that doesn't say anything about updating Project.toml
  • why is it updating the Project.toml?
  • how we can we stop it from doing that?
    • alternatively, one could of course also try to do any changes, like this: git checkout Project.toml

@gdalle
Copy link
Contributor Author

gdalle commented Jun 1, 2023

Even outside of CI, if the Project.toml has been formatted in Julia <= 1.6, the new version of Aqua will throw an error

@fingolfin
Copy link
Collaborator

Yeah, my central point is that the actual problem then is that something re-formatted Project.toml. It is bad in all Julia versions

  • in Julia <= 1.6 it breaks the test
  • in Julia >= 1.7 it prevents the test from breaking in many instances (i.e. if something is sorted incorrectly in Project.toml, you won't notice)

Now, what I still don't understand is why these updates happen. At the start of the GH Action julia-actions/julia-runtest I always see a message of the form

Updating `~/work/PACKAGENAME.jl/PACKAGENAME.jl/Project.toml`

which indicates something is going on; but I never see this in my local tests. Speaking from a purely logical point of view, I also don't understand why it would be updating Project.toml at that point -- all it should be doing is instantiating an environment (and thus updating/creating Manifest.toml). What am I missing?

@adrhill
Copy link

adrhill commented Sep 13, 2023

I'm running into the same issue.

JuliaLang/Pkg.jl#3481 mentions:

In order for it to be fixed in Julia there needs to be a new 1.6 release, it seems easier if possible to add a workaround in Aqua.

If I'm not mistaken, everyone using Aqua.jl's defaults on a LTS-compatible package with extensions must be running into this problem.

@gdalle
Copy link
Contributor Author

gdalle commented Sep 13, 2023

My solution is to only Aqua-test the package on v1.7 and above but it's not perfect. If you see a workaround, do speak up ;)

@adrhill
Copy link

adrhill commented Sep 13, 2023

This currently requires breaking up Aqua.test_all into individual tests.

Maybe on 1.6, the Project.toml could be checked for [weakdeps] and [extensions] and throw a warning instead of an error when running Aqua.test_project_toml_formatting?

Edit: Maybe Aqua.test_all(MyPackage; project_toml_formatting=VERSION >= v"1.7") is simple enough.

@lgoettgens
Copy link
Collaborator

This currently requires breaking up Aqua.test_all into individual tests.

No. You can do it like this:

Aqua.test_all(
    YourPackage;
    project_toml_formatting=(VERSION >= v"1.7")
)

@gdalle
Copy link
Contributor Author

gdalle commented Oct 12, 2023

But the issue remains that if CI modifies the Project.toml, this test will automatically pass even though it would have failed locally

@lgoettgens
Copy link
Collaborator

lgoettgens commented Oct 13, 2023

The initial issue has been indeed closed by #106. For the current discussion here, I rather have a new issue with a more prominent name (see #208).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants