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

Fixes for the conda-for-Sage-developers installation method, add GH Actions workflow #30845

Closed
mkoeppe opened this issue Nov 1, 2020 · 375 comments

Comments

@mkoeppe
Copy link
Member

mkoeppe commented Nov 1, 2020

Add github action workflow that checks the build of sage in a conda environment, completely bypassing the installation of any sage package. This is based on the steps outlined at https://wiki.sagemath.org/Conda.

This also tests that the generated src/environment*.yml files work correctly. (See documentation added in #28745)

Run: https://github.com/sagemath/sagetrac-mirror/actions/workflows/ci-conda.yml

Fixes:

  • Conda: Add primecountpy #33330: primecountpy is not installed in the conda env because its not listed in environment.yml
  • environment-optional: lcalc 1 is installed and then not recognized / leads to error
building 'sage.libs.lcalc.lcalc_Lfunction' extension
    cc1plus: warning: command line option '-Wstrict-prototypes' is valid for C/ObjC but not for C++
    In file included from sage/libs/lcalc/lcalc_Lfunction.cpp:740:
    sage/libs/lcalc/lcalc_sage.h:1:10: fatal error: lcalc/L.h: No such file or directory
        1 | #include "lcalc/L.h"
          |          ^~~~~~~~~~~
    compilation terminated.
  • environment-optional: probably related, pari 2.11.4 is installed by conda instead of the newest 2.13.2. This version is then rejected by configure as being too old.
  • Disable conda distro information for gdb because it is broken on macOS.
  • Add conda distro information for lrcalc_python
  • Filter out conda-specific ld warnings in doctests
  • In the feature test for sage_spkg, check whether any Sage packages are actually installed
  • Include an upper version bound for ptyprocess in sagemath-standard's install-requires

Follow-ups: See Meta-ticket #33331

Depends on #33358
Depends on #33330
Depends on #33361
Depends on #33141

CC: @isuruf @tobiasdiez @dimpase @saraedum

Component: build

Keywords: sd111

Author: Tobias Diez, Matthias Koeppe

Branch/Commit: 93fce5a

Reviewer: Matthias Koeppe, Dima Pasechnik, Tobias Diez

Issue created by migration from https://trac.sagemath.org/ticket/30845

@mkoeppe mkoeppe added this to the sage-9.3 milestone Nov 1, 2020
@mkoeppe mkoeppe changed the title GH Actions: Add test for conda without SPKG tox.ini, GH Actions: Add test for conda without SPKG Nov 3, 2020
@mkoeppe

This comment has been minimized.

@mkoeppe
Copy link
Member Author

mkoeppe commented Dec 6, 2020

comment:3

Hoping we can make progress on this ticket this week - https://wiki.sagemath.org/days111

@mkoeppe
Copy link
Member Author

mkoeppe commented Dec 6, 2020

Changed keywords from none to sd111

@mkoeppe

This comment has been minimized.

@tobiasdiez
Copy link
Contributor

comment:7

Something like https://github.com/tobiasdiez/sage/blob/public/build/conda_gh/.github/workflows/ci-conda.yml?

This currently fails with

configure: creating ./config.status
config.status: creating build/make/Makefile-auto
config.status: creating build/make/Makefile
config.status: creating src/bin/sage-env-config
config.status: creating src/bin/sage-src-env-config
config.status: creating build/bin/sage-build-env-config
config.status: creating build/pkgs/sage_conf/src/sage_conf.py
config.status: creating build/pkgs/sage_conf/src/setup.cfg
config.status: executing depfiles commands
config.status: executing mkdirs commands
config.status: creating directory /home/runner/work/sage/sage/logs/pkgs
config.status: creating directory 
mkdir: cannot create directory '': No such file or directory
config.status: error: could not create 
Error: Process completed with exit code 1.

https://github.com/tobiasdiez/sage/runs/1594237745?check_suite_focus=true
Any idea what could be the reason and how to fix it?

@tobiasdiez
Copy link
Contributor

Commit: 9af4614

@tobiasdiez
Copy link
Contributor

Author: Tobias Diez

@tobiasdiez
Copy link
Contributor

New commits:

c68d262Add first version of workflow
9af4614Try with sudo (why is this needed now?!)

@tobiasdiez
Copy link
Contributor

Branch: public/build/conda_gh

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Dec 22, 2020

Changed commit from 9af4614 to a178b56

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Dec 22, 2020

Branch pushed to git repo; I updated commit sha1. New commits:

5ac74eeTry with conda-incubator
a178b56Run in the conda shell

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Dec 22, 2020

Branch pushed to git repo; I updated commit sha1. New commits:

199886cInstall more conda dependencies
10de3f5Fix conda env name
86411b8Remove unnececesary install of conda dep

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Dec 22, 2020

Changed commit from a178b56 to 86411b8

@tobiasdiez
Copy link
Contributor

comment:11

Managed to fix the above error, just to be stuck two lines further down:

configure: creating ./config.status
config.status: creating build/make/Makefile-auto
config.status: creating build/make/Makefile
config.status: creating src/bin/sage-env-config
config.status: creating src/bin/sage-src-env-config
config.status: creating build/bin/sage-build-env-config
config.status: creating build/pkgs/sage_conf/src/sage_conf.py
config.status: creating build/pkgs/sage_conf/src/setup.cfg
config.status: executing depfiles commands
config.status: executing broken-gcc commands
config.status: executing mkdirs commands
config.status: creating directory /home/runner/work/sage/sage/logs/pkgs
config.status: creating directory /usr/share/miniconda/envs/sage-build
config.status: creating directory /usr/share/miniconda/envs/sage-build/bin
config.status: creating directory /usr/share/miniconda/envs/sage-build/etc
config.status: creating directory /usr/share/miniconda/envs/sage-build/include
config.status: creating directory /usr/share/miniconda/envs/sage-build/lib
config.status: creating directory /usr/share/miniconda/envs/sage-build/lib/pkgconfig
config.status: creating directory /usr/share/miniconda/envs/sage-build/share
config.status: creating directory /usr/share/miniconda/envs/sage-build/var/lib/sage/installed
config.status: error: Cannot perform incremental update, run "make distclean && make"
config.status: /usr/share/miniconda/envs/sage-build/lib64 is not a symlink, see Trac #19782
Error: Process completed with exit code 1.

https://github.com/tobiasdiez/sage/runs/1594601374?check_suite_focus=true

@tobiasdiez
Copy link
Contributor

comment:12

Found a hack that fixed it by removing the lib64 folder. It only contained libcc1.so (not sure where/how this is needed).

@tobiasdiez
Copy link
Contributor

comment:13

Made a bit more progress, but it currently errors while compiling gcc. Moreover, there is a problem in the detection of the conda/system gcc because "sage-env-config" doesn't exist.
https://github.com/tobiasdiez/sage/runs/1594867631?check_suite_focus=true

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Dec 22, 2020

Branch pushed to git repo; I updated commit sha1. New commits:

53503dcTry remove lib64 folder by hand
e530ac0Add c++ compiler to conda
ab47f5eInstall directly on top of conda
741c55aMake autogen work

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Dec 22, 2020

Changed commit from 86411b8 to 741c55a

@mkoeppe
Copy link
Member Author

mkoeppe commented Dec 22, 2020

comment:15

The direct variant should use src/environment.yml - which installs the Python packages of Sage in addition to what is listed in environment.yml

@mkoeppe
Copy link
Member Author

mkoeppe commented Dec 22, 2020

comment:16

The lib64 looks like a conda packaging bug to me - @isuruf, can you help?

@mkoeppe
Copy link
Member Author

mkoeppe commented Dec 22, 2020

comment:17

environment.yml already includes compilers -- I don't think adding cxx-compiler should be necessary

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Dec 22, 2020

Changed commit from 741c55a to 3aba048

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Dec 22, 2020

Branch pushed to git repo; I updated commit sha1. New commits:

1231a24Add env logging and fix SAGE_SRC
be024f8Fix build
8a46d04Improve output of print_env
9802c56Improve print statement
3aba048Use correct conda env file

@mkoeppe
Copy link
Member Author

mkoeppe commented Dec 22, 2020

comment:19

Replying to @tobiasdiez:

Made a bit more progress, but it currently errors while compiling gcc. Moreover, there is a problem in the detection of the conda/system gcc because "sage-env-config" doesn't exist.
https://github.com/tobiasdiez/sage/runs/1594867631?check_suite_focus=true

This is now #31097.

@mkoeppe
Copy link
Member Author

mkoeppe commented Dec 22, 2020

Changed dependencies from #28745 to #31097

@mkoeppe
Copy link
Member Author

mkoeppe commented Mar 18, 2022

comment:317

Nevertheless, continue-on-error makes no sense as I have explained throughout the thread, comment:141 etc.

@mkoeppe
Copy link
Member Author

mkoeppe commented Mar 18, 2022

comment:318

But it's fine for now, let's see that we can get it merged in 9.6.

@mkoeppe
Copy link
Member Author

mkoeppe commented Mar 18, 2022

comment:319

Onward to documenting it, #33426?

@mkoeppe
Copy link
Member Author

mkoeppe commented Mar 18, 2022

comment:321

Replying to @tobiasdiez:

they probably should have been extracted to their own tickets as they don't have to do anything with the github workflow

Yes, the fix to src/sage/features/pkg_systems.py could perhaps have been made a separate ticket and a dependency here. But it would have been difficult to test it separately because the present ticket is its only use case.

Sometimes the scope of a ticket needs to be adjusted during development. You already know that sometimes the scope turns out to be too big and it is necessary break out smaller, more manageable and more clearly defined tickets.

But sometimes the scope of a ticket also turns out to be too small and the ticket is not viable on its own. This is what happened on the present ticket. The workflow documented in the wiki https://wiki.sagemath.org/Conda has never actually worked. Creating a GH Actions workflow based on a semi-quasi-working variant of it, by itself was not convincing as a ticket (comment:121 - "what does it test"). With the broader scope of actually fixing this way of installing Sage and testing something that we can recommend to developers, the ticket has a viable scope: The changes bring a clear improvement to Sage.

@mkoeppe
Copy link
Member Author

mkoeppe commented Mar 18, 2022

comment:322

Some of the test suite failures that we saw are from ipython 8.x; #33170 updates the doctests

@tobiasdiez
Copy link
Contributor

comment:323

Replying to @mkoeppe:

But it's fine for now, let's see that we can get it merged in 9.6.

Thanks for the review!

@tobiasdiez
Copy link
Contributor

comment:324

Replying to @mkoeppe:

Onward to documenting it, #33426?

Yes!

@tobiasdiez
Copy link
Contributor

comment:325

Replying to @mkoeppe:

Replying to @tobiasdiez:

they probably should have been extracted to their own tickets as they don't have to do anything with the github workflow

Yes, the fix to src/sage/features/pkg_systems.py could perhaps have been made a separate ticket and a dependency here. But it would have been difficult to test it separately because the present ticket is its only use case.

Sometimes the scope of a ticket needs to be adjusted during development. You already know that sometimes the scope turns out to be too big and it is necessary break out smaller, more manageable and more clearly defined tickets.

But sometimes the scope of a ticket also turns out to be too small and the ticket is not viable on its own. This is what happened on the present ticket. The workflow documented in the wiki https://wiki.sagemath.org/Conda has never actually worked. Creating a GH Actions workflow based on a semi-quasi-working variant of it, by itself was not convincing as a ticket (comment:121 - "what does it test"). With the broader scope of actually fixing this way of installing Sage and testing something that we can recommend to developers, the ticket has a viable scope: The changes bring a clear improvement to Sage.

In general I agree that a ticket has to have a clear scope and improvement. For this ticket here, the github workflow essentially didn't change in the past 4 weeks, and at this point it was already clear that the workflow works in principle, with the caveat that a lot of docstests failed. In my opinion, it would have been better to merge the ticket already (with the few additional improvements to the workflow that we have discussed), then follow-up tickets could have easily improved different aspects of the conda environment, while the previous runs of the ci would have served as a baseline. Even thinks like the addition of the tests for macos could have been small follow-up tickets. I really think the tendency of the sage developers to create a lot of parallel, inter-dependent tickets makes reviewing harder and slows down the process. A more linear approach would increase the dev experience and velocity in my opinion.

Anyway, I'm happy that this ticket is finally ready to be merged.

@mkoeppe
Copy link
Member Author

mkoeppe commented Mar 18, 2022

comment:326

Replying to @tobiasdiez:

it was already clear that the workflow works in principle

The project generally uses a standard for tickets higher than that.
Yes, there's a tradeoff between velocity and stability.

@isuruf
Copy link
Member

isuruf commented Mar 21, 2022

comment:327

Is there a ticket for the remaining failures after this ticket?

@tobiasdiez
Copy link
Contributor

comment:328

Yes, they are collected in #33331.

@vbraun
Copy link
Member

vbraun commented Mar 27, 2022

Changed branch from public/build/conda-runci to 93fce5a

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

7 participants