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

Add compat entries for all deps (even stdlibs) #1554

Merged
merged 1 commit into from
Oct 18, 2023

Conversation

lgoettgens
Copy link
Collaborator

@codecov
Copy link

codecov bot commented Oct 17, 2023

Codecov Report

Merging #1554 (29c37ce) into master (110d3be) will decrease coverage by 0.07%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #1554      +/-   ##
==========================================
- Coverage   84.20%   84.14%   -0.07%     
==========================================
  Files          94       94              
  Lines       36602    36602              
==========================================
- Hits        30820    30798      -22     
- Misses       5782     5804      +22     

see 14 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@lgoettgens lgoettgens force-pushed the lg/compats branch 2 times, most recently from f947739 to d24c40b Compare October 17, 2023 11:58
RandomExtensions = "0.4.2"
SHA = "~1.6, ~1.7, 0.7"
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I know that this looks weird, but it is the only option as long as we support julia 1.6 and 1.7. See JuliaCrypto/SHA.jl#87

@@ -1,5 +1,4 @@
[deps]
AbstractAlgebra = "c3fe647b-3220-5bb0-a1ea-a7954cac585d"
Copy link
Member

Choose a reason for hiding this comment

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

Any reason to remove this? I keep adding it back when I build the documentation locally, which is why I finally committed it. Note that the CI job building the docs also adds it (we just don't commit there, obviously).

Copy link
Member

Choose a reason for hiding this comment

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

Oh wait, I misread this. What I mean is that I keep adding Nemo to docs/Project.toml... But I agree that AA should be removed here (with the other changes you made below this is possible)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Somewhere in the julia docs I read that the package itself shouldn't be j it's docs/test project deps.
But yeah, this is something different

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh wait, I misread this. What I mean is that I keep adding Nemo to docs/Project.toml... But I agree that AA should be removed here (with the other changes you made below this is possible)

CI seems to fail due to this. Any idea what's going wrong? I hoped that the change in the setdocmeta would be enough

Copy link
Member

Choose a reason for hiding this comment

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

Yeah I slowly start to remember, I've run into this before (probably more than once). As the error in the CI logs indicate, using AbstractAlgebra won't just silently proceed when there is already a module AbstractAlgebra present. Perhaps it works if you do using Nemo.AbstractAlgebra? But probably also not.... So either you need to hack the system to convince it that AA is already loaded. Or we change AA: Maybe we can change it to do using ..AbstractAlgebra in the individual jldoctest blocks, i.e.:

```jldoctest; setup = :(using ..AbstractAlgebra)

Empirical evidence why this might work: in a fresh Julia session:

julia> using AbstractAlgebra

julia> using ..AbstractAlgebra

In another fresh Julia session (with a different environment that only has Nemo but not AA):

julia> using Nemo

Welcome to Nemo version 0.37.1

Nemo comes with absolutely no warranty whatsoever


julia> using AbstractAlgebra
 │ Package AbstractAlgebra not found, but a package named AbstractAlgebra is available from a registry.
 │ Install package?
 │   (1.9) pkg> add AbstractAlgebra
 └ (y/n/o) [y]: n
ERROR: ArgumentError: Package AbstractAlgebra not found in current path.
- Run `import Pkg; Pkg.add("AbstractAlgebra")` to install the AbstractAlgebra package.
Stacktrace:
 [1] macro expansion
   @ ./loading.jl:1630 [inlined]
 [2] macro expansion
   @ ./lock.jl:267 [inlined]
 [3] require(into::Module, mod::Symbol)
   @ Base ./loading.jl:1611

julia> AbstractAlgebra
ERROR: UndefVarError: `AbstractAlgebra` not defined

julia> using Nemo.AbstractAlgebra

julia> AbstractAlgebra
AbstractAlgebra

julia> using AbstractAlgebra
 │ Package AbstractAlgebra not found, but a package named AbstractAlgebra is available from a registry.
 │ Install package?
 │   (1.9) pkg> add AbstractAlgebra
 └ (y/n/o) [y]: n
ERROR: ArgumentError: Package AbstractAlgebra not found in current path, maybe you meant `import/using .AbstractAlgebra`.
- Otherwise, run `import Pkg; Pkg.add("AbstractAlgebra")` to install the AbstractAlgebra package.
Stacktrace:
 [1] macro expansion
   @ ./loading.jl:1630 [inlined]
 [2] macro expansion
   @ ./lock.jl:267 [inlined]
 [3] require(into::Module, mod::Symbol)
   @ Base ./loading.jl:1611

julia> using ..AbstractAlgebra

Of course that would require making an AA PR now and then releasing it

Copy link
Member

Choose a reason for hiding this comment

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

Somewhere in the julia docs I read that the package itself shouldn't be j it's docs/test project deps. But yeah, this is something different

It would be interesting to know where you read this, and how whoever wrote it then builds their docs? Looking at [the Documenter.jl documentation](julia --project make.jl) they suggest to build package docs via

cd docs
julia --color=yes --project make.jl

Which of course only works if you just before did something like this:

julia --color=yes --project -e 'using Pkg ; Pkg.develop(path="..")'

which is precisely what I do (and essentially also what our CI does), except that then I have a diff in my Project.toml...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I really do not like that we need to change AA for this. Thus I would just re-add it to docs/Project.toml and deal with it sometime later.

docs/make.jl Outdated

DocMeta.setdocmeta!(Nemo, :DocTestSetup, :(using Nemo); recursive = true)
DocMeta.setdocmeta!(Nemo, :DocTestSetup, :(using Nemo; AbstractAlgebra = Nemo.AbstractAlgebra); recursive = true)
Copy link
Member

Choose a reason for hiding this comment

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

Not an objection, just a genuine question: I am wondering where we need this? Is it because of the doctests in AA which have setup = :(using AbstractAlgebra)? If so, is that the only reason? Perhaps we could add this as a comment before the setdocmeta!, for the next time I or someone else wonders about this ;-).

But if you don't know either, don't waste your time on this.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, that was my hope, but it doesn't work in the docs ci job.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'll try something else later today

@fingolfin fingolfin merged commit 4f99e9e into Nemocas:master Oct 18, 2023
14 of 15 checks passed
@lgoettgens lgoettgens deleted the lg/compats branch October 18, 2023 13:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants