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

Improve FLINT autogen README #37458

Merged
merged 5 commits into from
May 25, 2024
Merged

Conversation

grhkm21
Copy link
Contributor

@grhkm21 grhkm21 commented Feb 25, 2024

The FLINT headers are generated manually using a script from #36449. However, the documentations are not quite clear, and neither is the commit that the current headers are generated from. This commit adds these two details.

@videlec do you mind reviewing? I am not sure if the flint-commit.txt is an acceptable way to put this information, or where I should put it. Also I'm aware that this script might be moved in the future (since it shouldn't belong in sage_setup?) but that shouldn't conflict with this PR.

@videlec
Copy link
Contributor

videlec commented Feb 25, 2024

Thanks for improving the flint auto-generation. I will have a look.

@videlec
Copy link
Contributor

videlec commented Feb 25, 2024

I don't quite understand the point of the flint-commit.txt file. It is an extra burden to maintain. If you feel like the information is relevant, I would modify the script so that the (short) commit hash number is written inside each auto-generated file.

And in practice, it does not quite work this way. Typically, one has to modify flint sources so that the auto-generation works properly.

@grhkm21
Copy link
Contributor Author

grhkm21 commented Feb 25, 2024

Thanks for the response, the idea is that someone else should be able to reproduce the autogenerated headers on their machine too, and since e.g. for v3.0.1 a commit different from the release tag is used, it should be noted somewhere. I tried another approach, can you take a look? It puts the git commit line at the top of the generated files :)

@videlec
Copy link
Contributor

videlec commented Feb 25, 2024

Better this way I would say.

@grhkm21
Copy link
Contributor Author

grhkm21 commented Feb 25, 2024

Sorry there’s a small linting issue, I will fix it when I get back. @grhkm21

@grhkm21
Copy link
Contributor Author

grhkm21 commented Feb 25, 2024

Not sure why it got triggered, but the fix from https://stackoverflow.com/questions/30454549/a-literal-in-restructuredtext works. I assume this is still a positive review, feel free to revert if you disagree with the one-char change

@vbraun
Copy link
Member

vbraun commented Mar 26, 2024

merge conflict

Copy link

github-actions bot commented Mar 27, 2024

Documentation preview for this PR (built with commit 61674fd; changes) is ready! 🎉
This preview will update shortly after each push to this PR.

@vbraun vbraun merged commit b1d0e63 into sagemath:develop May 25, 2024
16 of 17 checks passed
@mkoeppe mkoeppe added this to the sage-10.4 milestone May 25, 2024
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.

4 participants