-
-
Notifications
You must be signed in to change notification settings - Fork 480
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
Replace bootstrap-conda by grayskull #37447
base: develop
Are you sure you want to change the base?
Conversation
I've removed |
do you mean Anyhow, after I do the latter, I get
What do I do wrong? Does it need to be run in a conda env? |
Sorry, forgot that the script uses conda-lock as well. Please try again after |
Why do I see on an amd64 box
while running |
I took the liberty to add the "needs work" label since it's unclear what's disputed here. (Also, you could maybe check or remove the checklist at the top of the PR to make it clear that there's nothing from that checklist missing here.) |
This comment was marked as abuse.
This comment was marked as abuse.
@saraedum @roed314 @jhpalmieri I set "disputed" here because when the PR was opened, I was not able to comment because Tobias had blocked me without explanation or giving notice to the sage-abuse committee. It appears that now I can comment, so I am removing "disputed". There has not been any meaningful review yet here, so conducting a vote would seem premature. |
It is correct that on my own PRs, @dimpase is blocked because of his persistent and aggressive code of conduct violations, including the type of denunciations that he has just written above. |
In any case, this PR here is based on #37446, which is in disputed status because of #37446 (comment) |
This comment was marked as abuse.
This comment was marked as abuse.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am giving it a positive review; I tried it out, I pointed a problem, the problem got fixed - despite a rather abusive "There has not been any meaningful review yet here, so conducting a vote would seem premature" comment.
Documentation preview for this PR (built with commit 55df9a9; changes) is ready! 🎉 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand that there are reservations about having the pyproject.toml
in the root directory. Otherwise, this PR does seem like a reasonable idea to me (replace a shell script with Python and relay some of the heavy lifting to an external project.)
Is there a better place for the pyproject.toml that would make this work for everybody?
@@ -0,0 +1,154 @@ | |||
#!/usr/bin/env python3 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be good to add an explanation at the top that explains what this script does. Or maybe add a --help
to the argument parser that prints such documentation.
tools/update-conda.py
Outdated
return dev_dependencies | ||
|
||
|
||
update_conda(Path(options.sourcedir)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not super important but if we put all the global bits of this script in a def main()
and do the usual if __name__ == "__main__"
or similar, then this could be imported by others to use bits of the machinery for other purposes.
Creating a new top-level And actually, there's no need for a new top-level directory: We already have a place for scripts of exactly this kind -- it is |
I agree, that script should probably just go elsewhere. But otherwise, if we somehow move the pyproject.toml file, I don't see a problem here. |
Same here. This could just be done on top of #37482. |
What's happening here? |
Using the metadata added in #37446, we automatically generate the conda environment files. This no longer makes any reference to the
conda.txt
files contained in sage-the-distribution. Thus sage-on-top-of-conda is now completely independent of sage-the-distribution (only relying on information specified by sage-the-library). In particular, after this PR is merged theconda.txt
files could be deleted from sage-the-distribution.In particular, we no longer need to maintain the mapping of pypi packages to conda packages but instead can rely on the offical mappings maintained by the conda team (https://github.com/regro/cf-graph-countyfair/tree/master/mappings/pypi).
To test:
As a byproduct, this fixes #34626.
📝 Checklist
⌛ Dependencies