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

build/pkgs/{givaro,fflas_ffpack,linbox}: Accept matching set of new versions #36997

Merged
merged 3 commits into from
Feb 13, 2024

Conversation

mkoeppe
Copy link
Contributor

@mkoeppe mkoeppe commented Jan 3, 2024

Resolves #34359.

📝 Checklist

  • The title is concise, informative, and self-explanatory.
  • The description explains in detail what this PR is about.
  • I have linked a relevant issue or discussion.
  • I have created tests covering the changes.
  • I have updated the documentation accordingly.

⌛ Dependencies

@tornaria
Copy link
Contributor

tornaria commented Jan 3, 2024

LGTM. I would eliminate the upper bounds, but don't really care.

@mkoeppe mkoeppe force-pushed the linbox_spkg_configure_versions branch from 02c0a4a to e3ac862 Compare January 14, 2024 21:43
@mkoeppe mkoeppe force-pushed the linbox_spkg_configure_versions branch from e3ac862 to d02e48e Compare January 22, 2024 01:06
@mkoeppe mkoeppe force-pushed the linbox_spkg_configure_versions branch from d02e48e to 9699e4c Compare February 2, 2024 23:21
Copy link

github-actions bot commented Feb 3, 2024

Documentation preview for this PR (built with commit 9699e4c; changes) is ready! 🎉

@mkoeppe mkoeppe requested a review from orlitzky February 8, 2024 18:51
@orlitzky
Copy link
Contributor

orlitzky commented Feb 9, 2024

LGTM if sage can actually use the newer versions (I don't have them available to test yet).

@tornaria
Copy link
Contributor

tornaria commented Feb 9, 2024

LGTM if sage can actually use the newer versions (I don't have them available to test yet).

It's been working for me since long time ago (and for arch even before).

Now that I'm testing sage-the-distro for #36181, I will merge this on top, test again, and report.

@tornaria
Copy link
Contributor

tornaria commented Feb 9, 2024

Clean workdir with a merge of #36181 (@ 29130ea) and this PR (@ 9699e4c). My script:

./bootstrap
./configure --disable-doc --disable-editable --enable-system-site-packages
MAKE='make -j36' make
./sage -tp 36 --all
./sage -tp 36 --all --long

Obtaining:

----------------------------------------------------------------------
sage -t --random-seed=179801755790120102275446393406114206910 src/sage/matroids/database_collections.py  # 4 doctests failed
----------------------------------------------------------------------
Total time for all tests: 208.9 seconds
    cpu time: 5923.8 seconds
    cumulative wall time: 5485.9 seconds

and

----------------------------------------------------------------------
sage -t --long --random-seed=285812227644826121201762562618130313328 src/sage/matroids/database_collections.py  # 7 doctests failed
----------------------------------------------------------------------
Total time for all tests: 386.5 seconds
    cpu time: 11481.1 seconds
    cumulative wall time: 11191.6 seconds

The failures in src/sage/matroids/database_collections.py are from #37140 so it's safe to ignore.

There's almost nothing installed in SAGE_LOCAL (in particular linbox et all are not -- they are really taken from the system and everything works):

$ ls -1 local/var/lib/sage/installed/
combinatorial_designs-20140630.p0
elliptic_curves-0.8.1
gnulib-f9b39c4e337f1dc0dd07c4f3985c476fb875d799
graphs-20210214.p0
jmol-14.29.52
polytopes_db-20170220.p0
threejs-r122.p0

So, LGTM.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Feb 9, 2024

Thanks for the review.

@vbraun vbraun merged commit d4b28e1 into sagemath:develop Feb 13, 2024
42 of 46 checks passed
@dimpase
Copy link
Member

dimpase commented Feb 14, 2024

This

db7703c47b27 (Matthias Koeppe  2024-01-02 18:54:05 -0800 11) ], [dnl REQUIRED_CHECK
db7703c47b27 (Matthias Koeppe  2024-01-02 18:54:05 -0800 12) ], [dnl PRE
db7703c47b27 (Matthias Koeppe  2024-01-02 18:54:05 -0800 13) ], [dnl POST
db7703c47b27 (Matthias Koeppe  2024-01-02 18:54:05 -0800 14)    sage_spkg_install_fflas_ffpack=$sage_spkg_install_linbox
db7703c47b27 (Matthias Koeppe  2024-01-02 18:54:05 -0800 15)    sage_spkg_install_givaro=$sage_spkg_install_linbox

has broken the case where givaro and/or fflas_ffpack from the system are OK, but linbox is not. So in effect it forces
the whole trio to either be from the system, or not. Why?

@mkoeppe mkoeppe deleted the linbox_spkg_configure_versions branch February 14, 2024 21:58
@mkoeppe
Copy link
Contributor Author

mkoeppe commented Feb 14, 2024

What do you mean when you say it has "broken" it?

@dimpase
Copy link
Member

dimpase commented Feb 14, 2024

If e.g. acceptable givaro is found on the system, but no acceptable linbox, then givaro still gets installed no matter what. This looks like a regression to me.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Feb 14, 2024

Are you working on an actual system where this worked and now it doesn't?

@dimpase
Copy link
Member

dimpase commented Feb 14, 2024

Are you working on an actual system where this worked and now it doesn't?

yes

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Feb 14, 2024

Details please (obviously).

@dimpase
Copy link
Member

dimpase commented Feb 14, 2024

OK, I now have all good packages, so config_ok.log is the result of such a ideal situation.
To show the breakage, I changed linbox's spkg-configure to ask for a non-existent version:

--- a/build/pkgs/linbox/spkg-configure.m4
+++ b/build/pkgs/linbox/spkg-configure.m4
@@ -4,7 +4,7 @@ SAGE_SPKG_CONFIGURE([linbox], [
       [linbox >= 1.6.3 linbox <= 1.6.4 fflas-ffpack >= 2.4.0 fflas-ffpack < 2.5.0 givaro >= 4.1.1 givaro < 4.2.0],
       [sage_spkg_install_linbox=no],
       [PKG_CHECK_MODULES([LINBOX],dnl Check for a set of matching new versions
-        [linbox >= 1.7.0 linbox <= 1.7.0 fflas-ffpack >= 2.5.0 givaro >= 4.2.0 givaro < 4.3.0],
+        [linbox >= 1.9.0 linbox <= 1.7.0 fflas-ffpack >= 2.5.0 givaro >= 4.2.0 givaro < 4.3.0],
         [sage_spkg_install_linbox=no],
         [sage_spkg_install_linbox=yes])])
   ])

here is the difference in config.logs.

$ diff -h config.log config_ok.log 
[... some noise removed ...]
< configure:38161: checking for         linbox >= 1.9.0 linbox <= 1.7.0 fflas-ffpack >= 2.5.0 givaro >= 4.2.0 givaro < 4.3.0
< configure:38168: $PKG_CONFIG --exists --print-errors "        linbox >= 1.9.0 linbox <= 1.7.0 fflas-ffpack >= 2.5.0 givaro >= 4.2.0 givaro < 4.3.0"
< Package dependency requirement 'linbox >= 1.9.0' could not be satisfied.
< Package 'linbox' has version '1.7.0', required version is '>= 1.9.0'
< configure:38171: $? = 1
< configure:38185: $PKG_CONFIG --exists --print-errors "        linbox >= 1.9.0 linbox <= 1.7.0 fflas-ffpack >= 2.5.0 givaro >= 4.2.0 givaro < 4.3.0"
< Package dependency requirement 'linbox >= 1.9.0' could not be satisfied.
< Package 'linbox' has version '1.7.0', required version is '>= 1.9.0'
< configure:38188: $? = 1
< configure:38202: result: no
< Package dependency requirement 'linbox >= 1.9.0' could not be satisfied.
< Package 'linbox' has version '1.7.0', required version is '>= 1.9.0'
< configure:38336: no suitable system package found for SPKG linbox
---
> configure:38161: checking for         linbox >= 1.7.0 linbox <= 1.7.0 fflas-ffpack >= 2.5.0 givaro >= 4.2.0 givaro < 4.3.0
> configure:38168: $PKG_CONFIG --exists --print-errors "        linbox >= 1.7.0 linbox <= 1.7.0 fflas-ffpack >= 2.5.0 givaro >= 4.2.0 givaro < 4.3.0"
> configure:38171: $? = 0
> configure:38185: $PKG_CONFIG --exists --print-errors "        linbox >= 1.7.0 linbox <= 1.7.0 fflas-ffpack >= 2.5.0 givaro >= 4.2.0 givaro < 4.3.0"
> configure:38188: $? = 0
> configure:38226: result: yes
> configure:38321: will use system package and not install SPKG linbox
[...]
< configure:124336: result: fflas_ffpack:                   no suitable system package; standard, SPKG version 2.4.3.p0 will be installed
---
> configure:124336: result: fflas_ffpack:                   using system package; SPKG will not be installed
7985c7979
< configure:129782: result: givaro:                         no suitable system package; standard, SPKG version 4.1.1 will be installed
---
> configure:129782: result: givaro:                         using system package; SPKG will not be installed
8058c8052
< configure:142586: result: linbox:                         no suitable system package; standard, SPKG version 1.6.3.p1 will be installed
---
> configure:142586: result: linbox:                         using system package; SPKG will not be installed
[...]

config.log

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Feb 14, 2024

So it works correctly on your system, and you are speculating about a broken configuration?

@dimpase
Copy link
Member

dimpase commented Feb 14, 2024

No, I'm not speculating, I am saying it's a regression introduced here to require building givaro if linbox must be built.
What's the point of

sage_spkg_install_givaro=$sage_spkg_install_linbox

?

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Feb 14, 2024

You know what to do: Open an Issue, describe the actual system configuration (not an imagined one), describe what happens, describe what should happen and why.

Be sure to read what is linked in the PR description.

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.

Merge givaro/fflas_ffpack/linbox detection into one spkg-configure.m4
5 participants