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

sagelib: Declare build system dependencies using src/pyproject.toml #30581

Closed
mkoeppe opened this issue Sep 16, 2020 · 25 comments
Closed

sagelib: Declare build system dependencies using src/pyproject.toml #30581

mkoeppe opened this issue Sep 16, 2020 · 25 comments

Comments

@mkoeppe
Copy link
Contributor

mkoeppe commented Sep 16, 2020

Even after #30580, setup.py still has an import-time dependency on Cython (via sage_setup).

We declare this build system dependency by adding the PEP 517 metadata (pyproject.toml).

Adding pyproject.toml does not change how the Sage distribution installs sagelib because build/pkgs/sagelib/spkg-install uses setup.py install directly.


References:

CC: @tobiasdiez @jhpalmieri @dimpase

Component: build

Keywords: sd111

Branch/Commit: u/mkoeppe/pyproject_toml @ bbfc19e

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

@mkoeppe mkoeppe added this to the sage-9.3 milestone Sep 16, 2020
@mkoeppe
Copy link
Contributor Author

mkoeppe commented Sep 16, 2020

Branch: u/mkoeppe/pyproject_toml

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Sep 16, 2020

Author: Matthias Koeppe

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Sep 16, 2020

Changed branch from u/mkoeppe/pyproject_toml to none

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Sep 16, 2020

Dependencies: #30580

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Sep 16, 2020

Branch: u/mkoeppe/pyproject_toml

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Sep 16, 2020

Commit: a05a537

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Sep 16, 2020

comment:4

Build fails with

    File "/private/var/folders/38/wnh4gf1552g_crsjnv2vmmww0000gp/T/pip-req-build-t5v70h8w/sage/misc/package.py", line 319, in installed_packages
      installed.update(pip_installed_packages())
    File "/private/var/folders/38/wnh4gf1552g_crsjnv2vmmww0000gp/T/pip-req-build-t5v70h8w/sage/misc/package.py", line 154, in pip_installed_packages
      for package in json.loads(stdout)}
    File "/usr/local/Cellar/[email protected]/3.8.5/Frameworks/Python.framework/Versions/3.8/lib/python3.8/json/__init__.py", line 357, in loads
      return _default_decoder.decode(s)
    File "/usr/local/Cellar/[email protected]/3.8.5/Frameworks/Python.framework/Versions/3.8/lib/python3.8/json/decoder.py", line 337, in decode
      obj, end = self.raw_decode(s, idx=_w(s, 0).end())
    File "/usr/local/Cellar/[email protected]/3.8.5/Frameworks/Python.framework/Versions/3.8/lib/python3.8/json/decoder.py", line 355, in raw_decode
      raise JSONDecodeError("Expecting value", s, err.value) from None
  json.decoder.JSONDecodeError: Expecting value: line 1 column 1 (char 0)
  ************************************************************************
  Error building the Sage library
  ************************************************************************

New commits:

2c1e648src/sage_setup/command/sage_build_cython.py: Remove top-level imports from sage.env, Cython
88c4e8csrc/setup.py: Make 'setup.py sdist' work without Cython
a05a537src/pyproject.toml: New

@mkoeppe

This comment has been minimized.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Oct 17, 2020

Changed dependencies from #30580 to #30580, #30712

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Nov 15, 2020

Changed dependencies from #30580, #30712 to #30580

@mkoeppe

This comment has been minimized.

@mkoeppe mkoeppe changed the title src/pyproject.toml Declare build system dependencies using src/pyproject.toml Nov 15, 2020
@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Nov 15, 2020

Changed commit from a05a537 to bbfc19e

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Nov 15, 2020

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

6fe4d56Merge branch 't/30779/duplicate_src_setup_py' into t/30580/sage_setup__remove_import_time_dependency___setup_requires___on__pkgconfig____numpy_
698a6easrc/sage_setup/command/sage_build_cython.py: Remove top-level imports from sage.env, Cython
bb32e80build/pkgs/sagelib/src/setup.py: Make 'setup.py sdist' work without Cython
15a6c2bMerge branch 't/30580/sage_setup__remove_import_time_dependency___setup_requires___on__pkgconfig____numpy_' into t/30581/pyproject_toml
310ebd1Merge tag '9.3.beta1' into t/30581/pyproject_toml
bbfc19ebuild/pkgs/sagelib/src: Apply PEP 517 changes

@mkoeppe

This comment has been minimized.

@mkoeppe mkoeppe changed the title Declare build system dependencies using src/pyproject.toml sagelib: Declare build system dependencies using src/pyproject.toml Nov 15, 2020
@tobiasdiez
Copy link
Contributor

Reviewer: Tobias Diez, ...

@tobiasdiez
Copy link
Contributor

comment:11

Is the following addition really necessary?

+# PEP 517 builds do not have . in sys.path
+sys.path.insert(0, os.path.dirname(__file__))

Not having . in the path is needed for the build isolation. (I'm fine with adding it for the moment and removing it in a follow-up ticket.)

Otherwise looks good to me!

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Nov 15, 2020

comment:12

I needed it when I first worked on this ticket; I'll double check if it's still necessary now. Thanks!

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Dec 6, 2020

comment:14

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

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Dec 6, 2020

Changed keywords from none to sd111

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jan 19, 2021

comment:15

Replying to @tobiasdiez:

Is the following addition really necessary?

+# PEP 517 builds do not have . in sys.path
+sys.path.insert(0, os.path.dirname(__file__))

Not having . in the path is needed for the build isolation. (I'm fine with adding it for the moment and removing it in a follow-up ticket.)

Yes, this is needed for finding sage.env, for example

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jan 19, 2021

Changed dependencies from #30580 to none

@mkoeppe

This comment has been minimized.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jan 21, 2021

Changed reviewer from Tobias Diez, ... to none

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jan 21, 2021

Changed author from Matthias Koeppe to none

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jan 21, 2021

comment:17

This ticket is hard to test separately, so I have merged it into #30913. We can close this one here.

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