Relativize more header imports #36754
Conversation
|
Thanks, should be fixed now. |
|
Documentation preview for this PR (built with commit 233a4ef; changes) is ready! 🎉 |
<!-- ^^^^^ 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 --> <!-- 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. --> Follow-up to sagemath#36598, relativizing the remaining header imports. ### 📝 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. --> - [ ] 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 <!-- List all open PRs that this PR logically depends on - sagemath#12345: short description why this is a dependency - sagemath#34567: ... --> <!-- If you're unsure about any of these, don't hesitate to ask. We're here to help! --> URL: sagemath#36754 Reported by: Tobias Diez Reviewer(s): Matthias Köppe
|
It is not working for me. I am not quite sure what it would need to work in fact A fundamental problem seems that the header was moved from the sdist source to the build directory instead of just copied I have no idea why. |
|
I had not realised that header was generated. The path to it in the build directory would need a |
|
The only way this work for you, is that you must be cythonising in the source tree instead of the build folder. Otherwise, |
|
I have the same issue as @kiwifb with 10.3.beta2. I also can't figure out where are the CI checks for this. This needs to be reverted! |
|
FWIW: The command line adds But here we don't know why you are making this change, since here you invoke #36598 and there you claim you "don't know why". What is wrong with the full path? |
|
Apparently it is related to the meson work. But the farey_symbol header is the only one for which things do not work in an out of source build. If meson can cope with this, it will have to wait for the switch in my opinion. In the meantime I am patching the latest beta. |
The "meson work" should not break building sagemath-standard. Please. And I don't see how the "meson work" is building sage: sage-the-distro should build sage-the-lib as packages (sagemath-standard, etc) otherwise it's like there is one way for "regular distros" and a different way for "sage the distro". Having special ways to build sage-the-lib for sage-the-distro feels like abusing monopoly power. Right now the (official?) only way I see to build sagemath-standard is to cd into pkgs/sagemath-standard and "python setup.py build". I think "python -m build" didn't work last time I tried. If sage_setup can be phased out in favour of meson-py building that is pep-517 compatible, that would be great. Standard tools instead of NIH: building the car not reinventing the wheel. Since I'm already ranting, lets get all the steam out: sage_conf is worse than useless, when I tried "pip install sage_conf" it didn't work because of python 3.12, but also it downloaded the whole sage source code into DOT_SAGE, wtf? I already dislike pip because all the mess it does on my filesystem, but this feels bad even for pip. TL;DR please don't break sagemath-standard. |
You will typically need |
I agree, and that's the plan for meson sketched in |
We don't have a CI check for building a wheel of sagemath-standard currently. We build wheels only of sagemath-objects and sagemath-categories (and some of the tiny optional distributions):
These distributions are not big enough to catch the reported failure in In #35095 (which currently still has ~50K lines of diff against develop), this is covered by the new distribution sagemath-schemes. Any help with upstreaming this work is very welcome. |
This wouldn't be a problem if sage-the-distro would eat its own dogfood. I mean, if sage-the-distro would install sage-the-lib in a standard pythonic way that can be used as an example by other distributions. As a matter of fact, However, |
It does, just not by default (which is the editable build via pkgs/sagemath-standard is used if you do |
Yes, that's that's the same as what I wrote above in #36754 (comment) regarding I think in your downstream packaging context, you'll never want to do anything without |
|
That there are 2 separate build systems for the Sage library is transitional and will go away when the modularization work is merged. |
|
One of the next steps in this direction: (help is welcome) |
This |
Yes, that's #35725 |
…ude` for `farey_symbol.h`
<!-- ^^^^^
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 reverts one change of sagemath#36754, which broke building sagemath-
standard as reported in
sagemath#36754 (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". -->
<!-- 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.
- [ ] 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: ...
-->
<!-- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->
URL: sagemath#36883
Reported by: Matthias Köppe
Reviewer(s):
I'm very sorry for the introduced bug. How do you run the compilation "out of the build" folder? Do you have any idea why only the farey symbol header makes problems but none of the other relative files?
You might be happy to hear that neither sage_conf nor sage_setup is needed with the meson compilation at #36524. In fact, it is completely independent of anything in "sage-the-distro". A simple |
That would be because it is the only header that is produced from a cython file. All the other files are present in the source tree. But |
|
Thanks, makes sense. I was able to find a workaround for the meson compilation. Sorry for the inconvenience |
Follow-up to #36598, relativizing the remaining header imports.
📝 Checklist
⌛ Dependencies