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 -fixdistributions #36135

Merged
merged 24 commits into from
Dec 6, 2023
Merged

Conversation

mkoeppe
Copy link
Contributor

@mkoeppe mkoeppe commented Aug 25, 2023

This is a new maintenance command for adding/updating # sage_setup: distribution directives at the top of source files.

Based on a discussion in #35884 (comment)

📝 Checklist

  • 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

@kwankyu
Copy link
Collaborator

kwankyu commented Aug 25, 2023

I understand the use case sage --fixdistributions --set DISTRIBUTION --from-egg-info. Do we have use cases with other set of switches? Would you list some of them? For instance,

sage --fixdistributions --set sagemath-objects 

is only dangerous.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Aug 25, 2023

I have added a bit of documentation. The manual versions of the commands are there simply because whenever an automatic mechanism is provided, there should also be a simpler, manual mechanism.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Aug 25, 2023

I've made it a bit safer.

@kwankyu
Copy link
Collaborator

kwankyu commented Aug 25, 2023

This

$ sage -fixdistributions --add=sagemath-categories --from-egg-info
Added 'sage_setup: distribution = sagemath-categories' directive in '/Users/kwankyu/GitHub/sage-dev/src/sage/all__sagemath_categories.py'
Added 'sage_setup: distribution = sagemath-categories' directive in '/Users/kwankyu/GitHub/sage-dev/src/sage/categories/additive_groups.py'
Added 'sage_setup: distribution = sagemath-categories' directive in '/Users/kwankyu/GitHub/sage-dev/src/sage/categories/additive_magmas.py'
...

is too verbose and repetitive. How about logging like

Added 'sage_setup: distribution = sagemath-categories' directive in 
'/Users/kwankyu/GitHub/sage-dev/src/sage/all__sagemath_categories.py'
'/Users/kwankyu/GitHub/sage-dev/src/sage/categories/additive_groups.py'
...

or something close.

@kwankyu
Copy link
Collaborator

kwankyu commented Aug 25, 2023

Similarly how about this?

$ sage --fixdistributions --set sagemath-objects src/sage/typeset/unicode_characters.py
Changed to 'sage_setup: distribution = sagemath-objects' directive from 'sage_setup: distribution = sagemath-categories' in
'src/sage/typeset/unicode_characters.py' 

@kwankyu
Copy link
Collaborator

kwankyu commented Aug 25, 2023

Perhaps because of --set case, my suggestion for --add may be infeasible...

@kwankyu
Copy link
Collaborator

kwankyu commented Aug 25, 2023

Is this meaningful?

$ sage --fixdistributions 
/Users/kwankyu/GitHub/sage-dev/src/sage/ext_data: non-package directory
/Users/kwankyu/GitHub/sage-dev/src/sage/all.py: file in distribution ''
/Users/kwankyu/GitHub/sage-dev/src/sage/all__sagemath_bliss.py: file in distribution ''
/Users/kwankyu/GitHub/sage-dev/src/sage/all__sagemath_categories.py: file in distribution 'sagemath-categories'
/Users/kwankyu/GitHub/sage-dev/src/sage/all__sagemath_coxeter3.py: file in distribution ''
/Users/kwankyu/GitHub/sage-dev/src/sage/all__sagemath_environment.py: file in distribution ''
/Users/kwankyu/GitHub/sage-dev/src/sage/all__sagemath_mcqd.py: file in distribution ''
/Users/kwankyu/GitHub/sage-dev/src/sage/all__sagemath_meataxe.py: file in distribution ''
/Users/kwankyu/GitHub/sage-dev/src/sage/all__sagemath_objects.py: file in distribution ''
/Users/kwankyu/GitHub/sage-dev/src/sage/all__sagemath_repl.py: file in distribution ''
/Users/kwankyu/GitHub/sage-dev/src/sage/all__sagemath_sirocco.py: file in distribution ''
/Users/kwankyu/GitHub/sage-dev/src/sage/all__sagemath_tdlib.py: file in distribution ''
/Users/kwankyu/GitHub/sage-dev/src/sage/all_cmdline.py: file in distribution ''
...

This is not a legitimate use, of course, but it would be better to guide erroneous people with a meaningful message.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Aug 26, 2023

Is this meaningful?

$ sage --fixdistributions 
/Users/kwankyu/GitHub/sage-dev/src/sage/ext_data: non-package directory
/Users/kwankyu/GitHub/sage-dev/src/sage/all.py: file in distribution ''
/Users/kwankyu/GitHub/sage-dev/src/sage/all__sagemath_bliss.py: file in distribution ''
/Users/kwankyu/GitHub/sage-dev/src/sage/all__sagemath_categories.py: file in distribution 'sagemath-categories'

Yes, without --set or --add, it's intended to print the distribution information

@kwankyu
Copy link
Collaborator

kwankyu commented Aug 26, 2023

Then does this

$ sage --fixdistributions src/sage/categories/action.pyx 
src/sage/categories/action.pyx: file in distribution ''

mean src/sage/categories/action.pyx is included in no distribution? Is that true, doesn't it belong to sagemath-objects?

I get the same result after make pypi-wheels

Is this documented?

@kwankyu
Copy link
Collaborator

kwankyu commented Aug 26, 2023

Now I understand its function

$ sage -fixdistributions --set sagemath-objects --from-egg-info
...
$ sage -fixdistributions src/sage/categories/action.pyx 
src/sage/categories/action.pyx: file in distribution 'sagemath-objects'

Nice. This is worth to be documented.

@kwankyu
Copy link
Collaborator

kwankyu commented Aug 26, 2023

(Hinted by the comment above) An alternative suggestion:

src/sage/categories/action.pyx: has no distribution 
src/sage/categories/action.pyx: has distribution 'sagemath-objects'
src/sage/categories/action.pyx: added distribution 'sagemath-objects'
src/sage/categories/action.pyx: changed to distribution 'sagemath-objects' from '...'

The advantage is that it is short and succinct, and shows the affected file first.

By the way, the concept of "empty distribution" (because you mention "nonempty distribution") is confusing. Is this

# sage_setup: distribution = 

empty distribution? Do we allow that? I think we should not.

@kwankyu
Copy link
Collaborator

kwankyu commented Aug 26, 2023

Now I understand why you allowed

$ sage -fixdistributions --set sagemath-objects some-file-or-directory

I guess it is because eventually manifest.in could be built from the distributions information of the files, so you want to update distribution info of the files manually. The switch --from-egg-info allows the flow of distributions info in the other direction. Nice. Am I right?

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Aug 27, 2023

An alternative suggestion:

src/sage/categories/action.pyx: has no distribution 
src/sage/categories/action.pyx: has distribution 'sagemath-objects'
src/sage/categories/action.pyx: added distribution 'sagemath-objects'
src/sage/categories/action.pyx: changed to distribution 'sagemath-objects' from '...'

The advantage is that it is short and succinct, and shows the affected file first.

Thanks for the suggestion. Done in 64e50f3

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Aug 27, 2023

I guess it is because eventually manifest.in could be built from the distributions information of the files, so you want to update distribution info of the files manually.

Yes, that's a possible future step

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Aug 27, 2023

the concept of "empty distribution" (because you mention "nonempty distribution") is confusing. Is this

# sage_setup: distribution = 

empty distribution?

I agree it is poor terminology. The empty string designates a catch-all distribution, likely sagemath-standard.

@kwankyu
Copy link
Collaborator

kwankyu commented Aug 27, 2023

Perhaps we may say "empty distribution (string) designates the standard distribution, which is likely to be sagemath-standard"...

You didn't answer, but I guess you don't allow literally

# sage_setup: distribution = 

but you meant no # sage_setup line at all by "empty distribution".

@kwankyu
Copy link
Collaborator

kwankyu commented Aug 27, 2023

How about adding in the help

By default, 'sage -fixdistributions' shows the distribution of each file. 
Distribution '' means the standard distribution (sagemath-standard).

or something?

@kwankyu
Copy link
Collaborator

kwankyu commented Aug 27, 2023

To prevent needless confusion, we may say "package" instead " packages" in

src/sage/libs/coxeter3: ordinary packages (with __init__.py) cannot be split in several distributions ('', 'sagemath-coxeter3')

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Aug 27, 2023

I guess you don't allow literally

# sage_setup: distribution = 

but you meant no # sage_setup line at all by "empty distribution".

It is allowed and is the same as no # sage_setup line.
The script creates such lines when using --set ''

@kwankyu
Copy link
Collaborator

kwankyu commented Aug 27, 2023

I get

$ sage --fixdistributions src/sage/monoids
src/sage/monoids/all.py: file in distribution ''
src/sage/monoids/automatic_semigroup.py: file in distribution ''
...
src/sage/monoids/trace_monoid.py: file in distribution ''
$ sage --fixdistributions src/sage/monoids/all.py 
Traceback (most recent call last):
  File "/usr/local/Cellar/[email protected]/3.10.9/Frameworks/Python.framework/Versions/3.10/lib/python3.10/runpy.py", line 196, in _run_module_as_main
    return _run_code(code, main_globals, None,
  File "/usr/local/Cellar/[email protected]/3.10.9/Frameworks/Python.framework/Versions/3.10/lib/python3.10/runpy.py", line 86, in _run_code
    exec(code, run_globals)
  File "/Users/kwankyu/GitHub/sage-dev/src/sage/misc/package_dir.py", line 548, in <module>
    handle_file(path)
TypeError: handle_file() missing 1 required positional argument: 'file

It should work also with a single file. Right?

@kwankyu
Copy link
Collaborator

kwankyu commented Aug 27, 2023

It is allowed and is the same as no # sage_setup line. The script creates such lines when using --set ''

OK, then.

@kwankyu
Copy link
Collaborator

kwankyu commented Aug 27, 2023

$ sage --fixdistributions --set sagemath-coxeter3 src/sage/libs/coxeter3
Traceback (most recent call last):
  File "/usr/local/Cellar/[email protected]/3.10.9/Frameworks/Python.framework/Versions/3.10/lib/python3.10/runpy.py", line 196, in _run_module_as_main
    return _run_code(code, main_globals, None,
  File "/usr/local/Cellar/[email protected]/3.10.9/Frameworks/Python.framework/Versions/3.10/lib/python3.10/runpy.py", line 86, in _run_code
    exec(code, run_globals)
  File "/Users/kwankyu/GitHub/sage-dev/src/sage/misc/package_dir.py", line 546, in <module>
    handle_file(root, file)
  File "/Users/kwankyu/GitHub/sage-dev/src/sage/misc/package_dir.py", line 521, in handle_file
    package_distributions_per_directives[root].add(distribution)
UnboundLocalError: local variable 'distribution' referenced before assignment

??

@mkoeppe mkoeppe force-pushed the sage_fixdistributions_command branch from 54e953d to f6fabd8 Compare November 16, 2023 00:20
@mkoeppe
Copy link
Contributor Author

mkoeppe commented Nov 16, 2023

Rebased

Copy link

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

@kwankyu
Copy link
Collaborator

kwankyu commented Nov 16, 2023

I get

$ ./sage --fixdistributions --set all --from-egg-info
sage --fixdistributions: found egg-info of distribution 'sagemath-bliss'
sage --fixdistributions: found egg-info of distribution 'sagemath-coxeter3'
sage --fixdistributions: found egg-info of distribution 'sagemath-mcqd'
sage --fixdistributions: found egg-info of distribution 'sagemath-meataxe'
sage --fixdistributions: found egg-info of distribution 'sagemath-sirocco'
sage --fixdistributions: found egg-info of distribution 'sagemath-tdlib'
sage --fixdistributions: found egg-info of distribution 'sagemath-environment'
src/sage/misc/package_dir.py: changed 'sage_setup: distribution' from 'sagemath-objects' to 'sagemath-environment'
sage --fixdistributions: found egg-info of distribution 'sagemath-categories'
sage --fixdistributions: found egg-info of distribution 'sagemath-repl'
sage --fixdistributions: found egg-info of distribution 'sagemath-objects'
src/sage/misc/package_dir.py: changed 'sage_setup: distribution' from 'sagemath-environment' to 'sagemath-objects'
sage --fixdistributions: checking consistency
src/sage/graphs/bliss_cpp: missing file all__sagemath_bliss.py
src/sage/doctest/tests: missing file all__sagemath_repl.py

Note the two lines beginning with src/sage/misc/package_dir.py: .... They seem to conflict to each other.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Nov 16, 2023

Note the two lines beginning with src/sage/misc/package_dir.py: .... They seem to conflict to each other.

Yes, some of the MANIFESTs are not clean yet (even in #35095). In --set all, the order matters. Sometimes one of the distributions claims a file but then another, listed later, corrects this claim.

Copy link
Collaborator

@kwankyu kwankyu left a comment

Choose a reason for hiding this comment

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

LGTM.

@kwankyu
Copy link
Collaborator

kwankyu commented Nov 16, 2023

Note the two lines beginning with src/sage/misc/package_dir.py: .... They seem to conflict to each other.

Yes, some of the MANIFESTs are not clean yet (even in #35095). In --set all, the order matters. Sometimes one of the distributions claims a file but then another, listed later, corrects this claim.

Yes. This is a useful feature of --fixdistributions, detecting conflicts.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Nov 16, 2023

Thank you!

vbraun pushed a commit to vbraun/sage that referenced this pull request Dec 4, 2023
sagemathgh-36135: `sage -fixdistributions`
    
<!-- ^^^^^
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 -->
This is a new maintenance command for adding/updating `# sage_setup:
distribution` directives at the top of source files.

Based on a discussion in
sagemath#35884 (comment)

<!-- 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". -->
- Part of: sagemath#29705
- Cherry-picked from: sagemath#35095
<!-- 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.
- [ ] 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#36533 (CI fix)

<!-- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->
    
URL: sagemath#36135
Reported by: Matthias Köppe
Reviewer(s): Kwankyu Lee, Matthias Köppe
@vbraun vbraun merged commit c1a3817 into sagemath:develop Dec 6, 2023
@mkoeppe mkoeppe added this to the sage-10.3 milestone Dec 6, 2023
@mkoeppe mkoeppe deleted the sage_fixdistributions_command branch January 16, 2024 05:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants