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.

Sign up for GitHub

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

Jump to bottom

Developer Guide: Improvements to explanation of version files #37401

Merged
merged 5 commits into from
Mar 31, 2024
Merged

Developer Guide: Improvements to explanation of version files #37401

merged 5 commits into from
Mar 31, 2024

Conversation

mkoeppe
Copy link
Contributor

@mkoeppe mkoeppe commented Feb 19, 2024 •
edited
Loading

📝 Checklist

Improvements prompted by the discussions in

All about install-requires.txt, package-version.txt, requirements.txt, ...

  • 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

@mkoeppe mkoeppe self-assigned this Feb 19, 2024
@mkoeppe mkoeppe requested a review from dimpase February 19, 2024 21:06
@mkoeppe
Copy link
Contributor Author

mkoeppe commented Feb 19, 2024 •
edited
Loading

@dimpase Does this edit answer your question in https://groups.google.com/g/sage-devel/c/MIU-xo9b7pc/m/9Zabp6mXAAAJ

@dimpase
Copy link
Member

dimpase commented Feb 20, 2024

No, one still doesn't see from this doc the need for install-requires.txt files.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Feb 20, 2024

How about now

@dimpase
Copy link
Member

dimpase commented Feb 20, 2024

ok, this is better.

@dimpase
Copy link
Member

dimpase commented Feb 20, 2024

why can't these files be renamed requirements.txt ? one can distinguish the package types by the presence of package-version.txt

Besides, it seems that the names of pypi packages can be better recorded in spkg names directly (perhaps subject to the usual - vs _ thing)

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Feb 20, 2024

why can't these files be renamed requirements.txt ? one can distinguish the package types by the presence of package-version.txt

Because requirements.txt means something else. It is the name one uses for version pinnings, as in pip install -r requirements.txt.

install-requires is the traditional name (in setuptools) for declaring dependencies.

Besides, it seems that the names of pypi packages can be better recorded in spkg names directly (perhaps subject to the usual - vs _ thing)

You mean:

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Feb 20, 2024

install-requires is the traditional name (in setuptools) for declaring dependencies.

However:

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Feb 20, 2024

@dimpase Positive review, or is more work needed here?

@mkoeppe mkoeppe requested a review from kwankyu February 23, 2024 05:39
@mkoeppe
Copy link
Contributor Author

mkoeppe commented Feb 23, 2024

@dimpase Setting back to "needs work" because of #36999 (comment), #36999 (comment), #36999 (review)

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Feb 23, 2024

@dimpase New material to review.

@mkoeppe mkoeppe requested a review from kwankyu February 23, 2024 17:51
@mkoeppe mkoeppe force-pushed the document_version_files_better branch from e3c513e to fdcaf18 Compare February 25, 2024 17:36
Matthias Koeppe added 2 commits March 2, 2024 16:35
@mkoeppe mkoeppe force-pushed the document_version_files_better branch from f0d3d04 to 8c5710d Compare March 3, 2024 00:35
@github-actions GitHub Actions
Copy link

github-actions bot commented Mar 3, 2024

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

NathanDunfield
NathanDunfield approved these changes Mar 3, 2024
View reviewed changes
Copy link
Contributor

@NathanDunfield NathanDunfield left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Mar 5, 2024

Thanks @NathanDunfield. I'll set it to positive review.

@vbraun vbraun merged commit 73fbd6f into sagemath:develop Mar 31, 2024
15 of 16 checks passed
@mkoeppe mkoeppe added this to the sage-10.4 milestone Mar 31, 2024
vbraun pushed a commit to vbraun/sage that referenced this pull request Apr 1, 2024
sagemathgh-36999: Rename install-requires.txt to `version_requireme…
4a17172 
…nts.txt`

    
<!-- ^^^^^
Please provide a concise, informative and self-explanatory title.
Don't put issue numbers in there, do this in the PR body below.
For example, instead of "Fixes sagemath#1234" use "Introduce new method to
calculate 1+1"
-->
<!-- Describe your changes here in detail -->

As discussed in sagemath#36982:
- the name "install-requires"  has become outdated with the transition
to the new names set by the `pyproject.toml` format (see https://setupto
ols.pypa.io/en/latest/userguide/dependency_management.html#declaring-
required-dependency, compare the tabs "pyproject.toml" vs. "setup.cfg")
- this will help with sagemath#35890, as
GitHub will be able to just read the `version_requirements.txt` files

sagemath#36982 (comment)

Notes for reviewers:
- **This is a simple, limited-scope improvement and not intended as a
long-term commitment to the format of the files in build/pkgs/....**
- PRs sagemath#37430, sagemath#37350, sagemath#36740 remove direct access to build/pkgs in favor
of using the sage_bootstrap API (sage --package).

<!-- Why is this change required? What problem does it solve? -->
<!-- If this PR resolves an open issue, please link to it here. For
example "Fixes sagemath#12345". -->
<!-- If your change requires a documentation PR, please link it
appropriately. -->

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->
<!-- If your change requires a documentation PR, please link it
appropriately -->
<!-- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->
<!-- Feel free to remove irrelevant items. -->

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

### ⌛ Dependencies

<!-- List all open PRs that this PR logically depends on
- sagemath#12345: short description why this is a dependency
- sagemath#34567: ...
-->
- Depends on sagemath#37401

<!-- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->
    
URL: sagemath#36999
Reported by: Matthias Köppe
Reviewer(s): Dima Pasechnik, Kwankyu Lee, Nathan Dunfield
vbraun pushed a commit to vbraun/sage that referenced this pull request Apr 8, 2024
sagemathgh-36999: Rename install-requires.txt to `version_requireme…
2f5d9f1 
…nts.txt`

    
<!-- ^^^^^
Please provide a concise, informative and self-explanatory title.
Don't put issue numbers in there, do this in the PR body below.
For example, instead of "Fixes sagemath#1234" use "Introduce new method to
calculate 1+1"
-->
<!-- Describe your changes here in detail -->

As discussed in sagemath#36982:
- the name "install-requires"  has become outdated with the transition
to the new names set by the `pyproject.toml` format (see https://setupto
ols.pypa.io/en/latest/userguide/dependency_management.html#declaring-
required-dependency, compare the tabs "pyproject.toml" vs. "setup.cfg")
- this will help with sagemath#35890, as
GitHub will be able to just read the `version_requirements.txt` files

sagemath#36982 (comment)

Notes for reviewers:
- **This is a simple, limited-scope improvement and not intended as a
long-term commitment to the format of the files in build/pkgs/....**
- PRs sagemath#37430, sagemath#37350, sagemath#36740 remove direct access to build/pkgs in favor
of using the sage_bootstrap API (sage --package).

<!-- Why is this change required? What problem does it solve? -->
<!-- If this PR resolves an open issue, please link to it here. For
example "Fixes sagemath#12345". -->
<!-- If your change requires a documentation PR, please link it
appropriately. -->

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->
<!-- If your change requires a documentation PR, please link it
appropriately -->
<!-- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->
<!-- Feel free to remove irrelevant items. -->

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

### ⌛ Dependencies

<!-- List all open PRs that this PR logically depends on
- sagemath#12345: short description why this is a dependency
- sagemath#34567: ...
-->
- Depends on sagemath#37401

<!-- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->
    
URL: sagemath#36999
Reported by: Matthias Köppe
Reviewer(s): Dima Pasechnik, Kwankyu Lee, Nathan Dunfield
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@NathanDunfield NathanDunfield NathanDunfield approved these changes

@dimpase dimpase dimpase approved these changes

@kwankyu kwankyu Awaiting requested review from kwankyu

@jhpalmieri jhpalmieri Awaiting requested review from jhpalmieri

@nbruin nbruin Awaiting requested review from nbruin

Assignees

@mkoeppe mkoeppe

Labels
c: documentation p: critical / 2
Projects
None yet
Milestone
sage-10.4
Development

Successfully merging this pull request may close these issues.
5 participants
@mkoeppe @dimpase @kwankyu @NathanDunfield @vbraun