-
-
Notifications
You must be signed in to change notification settings - Fork 213
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
Delay setting default Configuration root #702
Conversation
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.
at first glance this looks good, i suspect the the new rootfinding fails in certain cases on windows due to drive differences
FYI @GDYendell, I fixed the tests and added handling for a previously neglected edge case ( tests are passing. You can see the results on my fork: https://github.com/qubidt/setuptools_scm/actions/runs/2191926795 |
Thanks @qubidt ! @RonnyPfannschmidt shall I merge, or perhaps just cherry pick, these additional changes or do you want to take a look at it separately first? |
I just returned from a 2 week vacation and haven't gotten to this yet in detail I like the general idea, but have to verify some details on how to integrate things (the details around creating config objects are quite more messy than I'd like and your changes made that even more apparent) |
@RonnyPfannschmidt any update on considering this PR or an alternative solution to the issue? |
I'm currently just not able to commit as much time to setuptools _acm as I'd like to |
i finally did the review, indeed when root is overridden, then it should be ignored/not passed down the approach implemented here unfortunately is not entirely correct, but the deferring a a a concept is the right idea |
i cant reiterate this for the next release, but this is the next change i will sort out |
i started to recreate the behaviour starting with creating unitests for the cli - i hope i can complete that pr today, |
superseeds pypa#702 which served as inspiration adds integration tests
closing in favor of #731 which was heavily inspired by this one, but also adds tests and a warning for root enforcement (cli root superseeding config root) thanks for providing this initial version which greatly cut down the time i needed to understand the problem |
Thanks @RonnyPfannschmidt ! ⭐ |
I have experienced the same issue reported in #691. This change is intended to fix it.
As far as I can tell, in projects where
pyproject.toml
is not in the root,pip install
will not work withoutroot
(andrelative_to
) defined in thepyproject.toml
, but if it is defined inpyproject.toml
thenpython -m setuptools_scm
will not work - it gives a duplicate keyword argument error:The problem is that it falls back to the setting root to be the same directory as
pyproject.toml
if--root
is not passed as an option and passes that intoConfiguration.from_file
, soroot
appears in both**section
and**kwargs
incls()
call.This change moves the setting of this default down into
Configuration.from_file
, so that it can check if it already has a root from the config file. The effect is that--root
will supercede the config file, but if--root
is not passed and it exists in the config file, it will use that (where currently it throws an error). The behaviour of using--root
(or"."
if it is not given) to search for the config file and to create the default Configuration is maintained.Fixes #691