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

sage_setup: Add missing dependency #33983

Closed
mkoeppe opened this issue Jun 12, 2022 · 16 comments
Closed

sage_setup: Add missing dependency #33983

mkoeppe opened this issue Jun 12, 2022 · 16 comments

Comments

@mkoeppe
Copy link
Contributor

mkoeppe commented Jun 12, 2022

This is blocking the creation of sdists for PyPI

https://github.com/sagemath/sage/runs/6849918030?check_suite_focus=true

 File "/home/runner/work/sage/sage/local/var/lib/sage/venv-python3.8/lib/python3.8/site-packages/sage_setup/autogen/interpreters/utils.py", line 19, in <module>
    from jinja2 import Environment
ModuleNotFoundError: No module named 'jinja2'

CC: @kiwifb

Component: build

Author: Matthias Koeppe

Branch: 1176758

Reviewer: François Bissey

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

@mkoeppe mkoeppe added this to the sage-9.7 milestone Jun 12, 2022
@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jun 12, 2022

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jun 12, 2022

Commit: d490b8f

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jun 12, 2022

New commits:

d490b8fbuild/pkgs/sage_setup/dependencies: Add jinja2

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jun 12, 2022

Author: Matthias Koeppe

@kiwifb
Copy link
Member

kiwifb commented Jun 12, 2022

comment:3

I'll say that's damning evidence. I did a quick inspection to see if something else obvious was missing but I couldn't find anything. Shouldn't it also be in requirements.txt and setup.cfg? Especially if it blocks stuff on Pypi, I would expect those to be looked at rather than dependencies which is a sage build system artifact not a pure python one.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 12, 2022

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

1176758pkgs/sage-setup: Add jinja2 dep to requirements.txt, setup.cfg

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 12, 2022

Changed commit from d490b8f to 1176758

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jun 12, 2022

comment:5

Replying to @kiwifb:

Shouldn't it also be in requirements.txt and setup.cfg?

Indeed. Thanks!

Especially if it blocks stuff on Pypi, I would expect those to be looked at rather than dependencies which is a sage build system artifact not a pure python one.

We do build our sdists for PyPI by going through the Sage build system

@kiwifb
Copy link
Member

kiwifb commented Jun 12, 2022

Reviewer: François Bissey

@kiwifb
Copy link
Member

kiwifb commented Jun 12, 2022

comment:6

LGTM. Glad that I put a little bit more thought in that review.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jun 13, 2022

comment:7

Thanks!

@vbraun
Copy link
Member

vbraun commented Jun 13, 2022

Changed branch from u/mkoeppe/sage_setup__add_missing_dependency to 1176758

@kiwifb
Copy link
Member

kiwifb commented Jun 13, 2022

comment:9

In between this ticket and my previous one about the build target that was also related to autogen, it occured to me that the whole autogen should probably moved somewhere else in the near future.

I should expand, with modularization sage_setup is providing some build tools for other sage modules and possibly downstream package. autogen is only used by one of these modules at most and should therefore live with it. It is not at the present a common infrastructure used by many modules. May be you already have a ticket about this that I am unaware off?

@kiwifb
Copy link
Member

kiwifb commented Jun 13, 2022

Changed commit from 1176758 to none

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jun 13, 2022

comment:10

Yes, this is a good point. I haven't worked on it yet because I'm not 100% sure whether all of the "interpreters" that are built by autogen will end up in the same distribution package.

@kiwifb
Copy link
Member

kiwifb commented Jun 13, 2022

comment:11

That's a fair point. But I am happy that it is on your mind as well.

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

3 participants