Opened 2 years ago
Closed 2 years ago
#30088 closed defect (fixed)
Fix "make dist", add test run to GitHub Actions
Reported by:  Matthias Köppe  Owned by:  

Priority:  critical  Milestone:  sage9.2 
Component:  scripts  Keywords:  
Cc:  Volker Braun, John Palmieri, Michael Orlitzky  Merged in:  
Authors:  Matthias Koeppe  Reviewers:  Michael Orlitzky, John Palmieri 
Report Upstream:  N/A  Work issues:  
Branch:  160862f (Commits, GitHub, GitLab)  Commit:  160862fb38046c22a4798f9aa00c6cad4415f540 
Dependencies:  Stopgaps: 
Description (last modified by )
make dist
is currently broken.
https://github.com/mkoeppe/sage/runs/848070855
Finished cloning Sage sources make[1]: Entering directory '/home/runner/work/sage/sage/build/make' env SAGE_INSTALL_FETCH_ONLY=yes make B SAGERUNTIME= \ alabaster appnope arb babel bleach boost_cropped brial bzip2 cddlib c.... ... cd '/home/runner/work/sage/sage/build/pkgs/sage_conf' && . '/home/runner/work/sage/sage/src/bin/sageenv' && . '/home/runner/work/sage/sage/build/bin/sagebuildenvconfig' && sagelogger p '/home/runner/work/sage/sage/build/pkgs/sage_conf/spkginstall' '/home/runner/work/sage/sage/logs/pkgs/sage_confnone.log' [sage_confnone] Installing [sage_confnone] Error: Tried to use Sage's Python which was not yet installed. [sage_confnone] If this was called from an spkginstall script for another [sage_confnone] package you should add $(PYTHON) as a dependency in [sage_confnone] build/pkgs/<pkg>/dependencies [sage_confnone] Error: could not determine package name [sage_confnone] ******************************************************************************** [sage_confnone] Error installing [sage_confnone] *
On this ticket we fix it and add a test run to GH Actions so that it does not get broken again.
Related:
 #29896  make download
Change History (20)
comment:1 Changed 2 years ago by
Description:  modified (diff) 

comment:2 Changed 2 years ago by
Cc:  John Palmieri Michael Orlitzky added 

comment:3 Changed 2 years ago by
Description:  modified (diff) 

Priority:  major → critical 
Summary:  Fix "make sdist", add test run to GitHub Actions → Fix "make dist", add test run to GitHub Actions 
comment:4 Changed 2 years ago by
Branch:  → u/mkoeppe/fix__make_sdist___add_test_run_to_github_actions 

comment:5 Changed 2 years ago by
Commit:  → a17c655a2aac9c80a52893c5373805a233731186 

comment:7 Changed 2 years ago by
sage_conf
currently is the only package that is type=standard
, source=script
.
All other source=script
packages are type=optional
.
comment:8 Changed 2 years ago by
Commit:  a17c655a2aac9c80a52893c5373805a233731186 → de6464db6b94fcebd5f88337681994fc0eb07e4b 

comment:10 Changed 2 years ago by
Status:  new → needs_review 

comment:11 Changed 2 years ago by
Branch:  u/mkoeppe/fix__make_sdist___add_test_run_to_github_actions → u/mjo/ticket/30088 

Commit:  de6464db6b94fcebd5f88337681994fc0eb07e4b → 775b7f7a8a406f9ae8aa9ef542384c060b4cbbd4 
OK with an added comment? Otherwise, LGTM.
New commits:
775b7f7  Trac #30088: add a few explanatory comments to m4/sage_spkg_collect.m4.

comment:13 Changed 2 years ago by
Re "the only standard script package is sage_conf": sagelib is also a script package, isn't it? Or is that changing? (It is listed in build/bin/Makefile
under SCRIPT_PACKAGES
.)
comment:14 Changed 2 years ago by
You are right, I forgot that the ticket that made sagelib a script package was already merged.
comment:15 Changed 2 years ago by
Branch:  u/mjo/ticket/30088 → u/mkoeppe/ticket/30088 

comment:16 Changed 2 years ago by
Commit:  775b7f7a8a406f9ae8aa9ef542384c060b4cbbd4 → 160862fb38046c22a4798f9aa00c6cad4415f540 

The successful test run of the new dist
workflow job can be seen at https://github.com/mkoeppe/sage/runs/864159160
It is possible that I may need to make some adjustments to the new localmacosnohomebrewnobootstrap
tests, but I'll do that in #29929 as a follow up.
OK to set it to positive review?
New commits:
160862f  m4/sage_spkg_collect.m4: Mention sagelib in the comment

comment:17 Changed 2 years ago by
Reviewers:  → Michael Orlitzky 

Status:  needs_review → positive_review 
Ok, you fixed the comment already. Yeah it's good now. John, please add yourself as a reviewer if you want some credit/blame here.
comment:18 Changed 2 years ago by
Reviewers:  Michael Orlitzky → Michael Orlitzky, John Palmieri 

I will confirm that, not only does mske dist
work, but I turned off the internet on my computer and built Sage successfully from that tarball.
comment:20 Changed 2 years ago by
Branch:  u/mkoeppe/ticket/30088 → 160862fb38046c22a4798f9aa00c6cad4415f540 

Resolution:  → fixed 
Status:  positive_review → closed 
Could you leave a comment in there explaining what's going on? It just took me about 20 minutes to figure out that this was failing on sage_conf because it's a standard package (which are usually incuded in SAGE_SDIST_PACKAGES) but not one whose sources need to be downloaded before being included with the source tarball (which is what the misnomer SAGE_SDIST_PACKAGES actually contains a list of). All script/pip packages should be similar, according to this change.
Also, per the description, I think we need a tox thingy.