-
-
Notifications
You must be signed in to change notification settings - Fork 491
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
Revert PR with disputed dependencies #37796
Conversation
This PR should not be merged because it is obviously not an improvement. |
There was absolutely nothing inappropriate in Volker merging the PR that he merged. He's the release manager. |
Perhaps the people who were against #36676 and #36951 would disagree; I'd like to hear from them. I'm creating this in support of the process we have in place for resolving disputed PRs, which got mangled this time (we're still working out how it needs to coordinate with dependencies). I'm open to other suggestions for how to proceed, from the people who were objecting to the code in #36676 and #36951 that got accidentally merged in #36964. In the meantime, I'm going to sleep. |
Easy suggestion: How about doing nothing rather than creating additional damage? |
I don't think this PR is about the question of improvement or not. It's entirely a procedural question. When people voted on #36964, they did not vote on the dependencies of it. After all, the missing dependencies did not have the sufficient majorities to be set to positive review yet. Also, the diff could not even be viewed on GitHub, so I am sure most people voted on the description and discussion of the PR, not on the actual change set ("The diff you're trying to view is too large. We only load the first 3000 changed files.")
I don't think it was ever claimed that @vbraun did anything wrong here. It was just a procedural misunderstanding. He simply missed the message about not merging this yet (and nobody is blaming him for that.) The (disputed) PR mechanism needs a bit of fine-tuning to take dependencies into account. But that should probably be discussed on sage-devel.
I believe that not merging this is harmful for the community because it ignores the votes on those dependencies. I volunteer to create PRs to restore the state of the disputed dependencies and #36964 itself. |
Documentation preview for this PR (built with commit f16eb4b; changes) is ready! 🎉 |
I verified this PR by running Edit: not that I mistrust @roed314 in any way. But trust is not really transitive, a review is supposed to be independent of the author, and it makes no sense for anybody to read the diff. |
That's exactly the question, and the only question that is part of our project's review procedures. |
This cannot be merged. Enough is enough. |
Unbelievable. |
@mkoeppe you made no objection to Julian's comment "@vbraun please do not merge this one yet. It depends on two disputed PRs that have not received positive review yet." #36964 should have not been given a positive review because of its dependencies. This corrects that problem. |
That's not how it works. Our review procedures do not have special rules for PRs that "only revert a PR". |
+1 from me. |
This PR has not even been reviewed. It cannot even be voted on. |
@tornaria checked that it did a very simple task: revert another PR that was accidentally merged. That's a review. This is just reverting state back to what develop was yesterday. |
We don't merge PRs that are not improvements. Our review criteria are clear regarding this. |
Hello, So, let me see if I can summarize the situation correctly:
Before any further discussion occurs, I believe it would be a good thing that the points above shall be double-checked. Then the following things shall be clarified (my question might sound naive, but I would like clear answers):
Before that, I do not see how a clear decision can be taken of the present PR. |
+1 to merge this |
For the record, the vote on sage-devel was 8-4 in favor of this PR. I'm setting this back to positive review. |
@roed314 For the record, I consider it a spoiled ballot because of (1) the misleading title of the poll (https://groups.google.com/g/sage-devel/c/glI1YgArcXc/m/U0B_YF8hBAAJ); (2) the adversarial haste to revert these positively reviewed PRs; (3) the continued denial that violations of our reviewing code put these PRs into "disputed" status; (4) the failure to repair the damage inflicted by sage-devel posts that may have intimidated voters; just deleting them is insufficient. |
Merging this has also backed out #36951, which has the necessary votes as pointed out in #37796 (comment) |
In #36951 (comment) you did not count my -1. Please see: #36951 (comment) In any case, if you can rebase that on current develop, we can have a look. I agree on moving to |
Once more, Gonzalo, setuptools does not support this. What you keep declaring as the "correct meaning" of PEP 517/518, even after I explained it to you, is simply not a thing in actual Python packaging practice. |
Here's the supported and recommended way to customize the build dependencies: |
Of course, by declaring and implementing one's own build backend instead of |
It's just writing a function that returns a list of dependencies. This is not implementing a build backend, and the code is all spelled out in the link above (except for the But moreover, since this would be a python function, it would also allow us to create the build requirements dynamically, which seems like a big advantage. Currently the build requirements are determined when one runs the
But in fact, step 1 could also be done in python in |
Also an obvious big disadvantage, namely that the dependency is no longer visible in the metadata file. It's running counter to the trend to move this kind of stuff from setup.py (= available at runtime of the build) to a static file. |
Sure it could. Hard to see how that would possibly be an improvement though. The current design keeps the reference to the centralized version metadata strictly in the bootstrapping step of our monorepository, and after this we have clean source trees that do not make a reference to such things. |
That's a good point, since we would want the code to be written only once (in sage-setup) but each package have its own set of dependencies. Maybe this information can be stored in |
I think that reducing the dependencies for the "build_sdist" step is a micro-optimization that is not worth spending time on (or I would probably already have done it). |
There are branches |
Please go ahead, Julian. |
Thanks @saraedum. |
<!-- ^^^^^ 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 noted in sagemath#36951 (comment), the fact that the dependency declaration of sage-the-library depends on information contain in sage-the-distro's build folder is problematic for the separation of concerns. Sage-the-distro should rely on information of sage-the-library, not the other way around. Here we fix this for the build requirements by making `src/pyproject.toml` the single source of thruth. The corresponding `install-requires.txt` are demoted to mere indicator files (to be removed later). Moreover, this change allows us to make `pyproject.toml` a static file (no longer generated by configure), which should help with downstream packaging. --- Happy new year! <!-- 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. --> - [ ] 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 - sagemath#37796 <!-- 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#36982 Reported by: Tobias Diez Reviewer(s): Dima Pasechnik, Gonzalo Tornaría, Matthias Köppe, Tobias Diez
#36964 was inappropriately merged, since two dependencies were still disputed. This PR attempts to revert the merge. It was created by
📝 Checklist
⌛ Dependencies
None, though it will also revert #36676 and #36951.