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

Upgrade eclib to 20221012 #35067

Merged
merged 1 commit into from
Mar 13, 2023
Merged

Conversation

alexjbest
Copy link
Contributor

πŸ“š Description

πŸ“ Checklist

  • I have made sure that the title is self-explanatory and the description concisely explains the PR.
  • I have linked an issue or discussion.
  • I have created tests covering the changes.
  • I have updated the documentation accordingly.

βŒ› Dependencies

@alexjbest
Copy link
Contributor Author

I'm not sure if checking PKG_CHECK_MODULES([ECLIB], [eclib = SAGE_ECLIB_VER], is the right thing to be doing going forward, perhaps allowing users to have a newer eclib than sage would be better (especially for people getting dependencies from distributions)

@mkoeppe
Copy link
Member

mkoeppe commented Feb 10, 2023

A discussion on this topic took place in the last upgrade ticket, #34029 (comment) ff.

@alexjbest
Copy link
Contributor Author

alexjbest commented Feb 10, 2023

A discussion on this topic took place in the last upgrade ticket, #34029 (comment) ff.

Thanks, that seems pretty conclusive that we should just not be flexible with system eclib versions for now.

This then raises the question of whether we should pin the correct version in the distros/conda.txt (or similar), is that discussed somewhere?

@codecov-commenter
Copy link

Codecov Report

Base: 88.59% // Head: 88.59% // Increases project coverage by +0.00% πŸŽ‰

Coverage data is based on head (52b23a4) compared to base (104dde9).
Patch has no changes to coverable lines.

Additional details and impacted files
@@           Coverage Diff            @@
##           develop   #35067   +/-   ##
========================================
  Coverage    88.59%   88.59%           
========================================
  Files         2136     2136           
  Lines       396142   396142           
========================================
+ Hits        350948   350959   +11     
+ Misses       45194    45183   -11     
Impacted Files Coverage Ξ”
src/sage/interfaces/qsieve.py 71.30% <0.00%> (-2.61%) ⬇️
src/sage/rings/polynomial/pbori/parallel.py 59.82% <0.00%> (-1.79%) ⬇️
src/sage/modular/hecke/algebra.py 94.65% <0.00%> (-1.07%) ⬇️
src/sage/graphs/generators/random.py 91.26% <0.00%> (-0.86%) ⬇️
src/sage/graphs/generic_graph.py 89.12% <0.00%> (-0.42%) ⬇️
src/sage/schemes/elliptic_curves/hom_velusqrt.py 94.09% <0.00%> (-0.40%) ⬇️
src/sage/quadratic_forms/ternary_qf.py 66.92% <0.00%> (-0.39%) ⬇️
src/sage/modular/overconvergent/hecke_series.py 98.76% <0.00%> (-0.31%) ⬇️
src/sage/sandpiles/sandpile.py 90.92% <0.00%> (-0.07%) ⬇️
src/sage/numerical/interactive_simplex_method.py 91.84% <0.00%> (ΓΈ)
... and 12 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

β˜” View full report at Codecov.
πŸ“’ Do you have feedback about the report comment? Let us know in this issue.

@mkoeppe
Copy link
Member

mkoeppe commented Feb 11, 2023

This then raises the question of whether we should pin the correct version in the distros/conda.txt (or similar), is that discussed somewhere?

This is a good question. grep "" build/pkgs/*/distros/conda.txt shows that we have only very few version pins in our conda information. Generally more flexibility is better so that one does not run into conflicting version pins when setting up the environment using our generated environment files. (Moreover, in our installation mode Using conda to provide all dependencies for the Sage library (experimental), we do not build any non-Python packages itself, so there's no recourse when conda doesn't provide the correct version.)

However, conda search eclib reveals that eclib 20221012 is already available; and also it seems that versions going back to 2017 are still available; so I would think that pinning is unproblematic here.

@kiwifb
Copy link
Member

kiwifb commented Feb 13, 2023

For the record, I my hand was forced to use that version of eclib in sage-on-gentoo for both sage 9.7 and now sage 9.8. I have yet to hear or see failure reports.

@kiwifb kiwifb self-requested a review February 13, 2023 22:38
Copy link
Member

@kiwifb kiwifb left a comment

Choose a reason for hiding this comment

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

I am approving this.

Copy link
Member

@dimpase dimpase left a comment

Choose a reason for hiding this comment

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

lgtm

@vbraun vbraun merged commit d11cbc0 into sagemath:develop Mar 13, 2023
@mkoeppe mkoeppe added this to the sage-10.0 milestone Mar 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants