-
-
Notifications
You must be signed in to change notification settings - Fork 308
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
Prevent matching bogus parent gitignores #1643
Conversation
3e12180
to
891d20a
Compare
891d20a
to
10d2308
Compare
@ofek Think you could take a look at this? |
FYI tests on master are currently broken same as here so I don't think this is introducing any regressions. |
Hi there, I just spent some time on trying to fix my project not to produce empty wheels until I stumbled over this issue. Why I am writing here is because it was surprising me that such simple condition (at least as appearing to me) like "do not cross project root when scanning for certain files" really is a concern here. My first thought was: Wouldn't it be enough to simply stop scanning upwards the file tree when we encounter a Markus |
Technically that would also prevent the issue I was seeing...however that would differ from git's normal gitignore behavior so it may not be desirable.
If the python project root is in a subdirectory of a git project root directory it's possible that someone may want to use the gitignore above the python project root subdirectory but within the git project root directory to ignore specific files, so there may be a use case here for wanting to try and replicate git's behavior more closely. The logic of this check as I implemented it should only filter out the use of gitignore in cases where the python project root directory is itself entirely ignored(a case where it is effectively guaranteed that the python project is not tracked by git at all), while still allowing for files within the project root directory to be ignored if needed using gitignores above the project root. |
I see that there are valid use cases for having a relevant In my case, the situation was a bit special, since the python project I was working on was completely unrelated to any VCS. It was just a random project sitting in my user's home. Ages ago, I decided (and almost forgot about) to put some of the config files in my home under version control with git (literally making my whole home a git repo). Now, to not have random stuff under version control, I used kind of inverse git-ignore logic with a
I absolutely admit that this is a very rare case, and I also know that there are better/other ways today to manage user configuration. I just wanted to point out that there might be use cases that involve What if in some completely unrelated
I assume this still would break things. What would have helped me a lot to spot the issue in a straighter fashion:
|
This is designed to handle cases like that, it's designed for really any case where the project root is itself ignored. Does it not work for you?
I mean, that's effectively what this was designed for, in my case I have a fully ignored python project root build directory inside a meta build system.
Yeah, it's not very obvious, so much so that this sort of behavior of matching bogus parent gitignore files was even independently re-implemented in virtually every newer pep517 build backend in some form or another. I managed to fix most of them already but hatch and flit(but not flit-core so there's much less real world issues for downstream integrators) AFAIU are the only ones remaining with fixes pending for this type of bug.
If the project root is ignored then this change should disable the use of |
Up to know, I just assumed that it would work 🙂. But to confirm that, I ran a quick test by installing your version directly from git inside a virtualenv.
As you can see, there are no package files contained in the wheel (there should be a package named If I either remove my
in
It looks like your fix indeed does not work for my case. Did I miss something? |
Hmm, try with |
I think this is your issue, you forgot to actually install my change from this PR, the change is part of the I tried replicating your environment and it seems that was the issue, also I'm not sure which Currently doesn't work:
Works:
|
Thanks for testing with respect to my setup. I fixed my test environment and now have:
I installed in editable mode from a local working copy in order to be able to quickly switch branches. Hereby I can confirm:
I verified that none of the both calls above works when running from I investigated a bit more what code actually runs when using
In my estimation, using |
Ok, it took me a while, but here are my findings. The reason why I did not dig deeper into the code of As a test, I forcibly installed your patched version of As a conclusion:
Update: Just after writing this, I found the following way to make Set the following in your [build-system]
requires = ["hatchling @ file:///path/to/your/local/hatch/backend"]
build-backend = "hatchling.build" |
10d2308
to
253a549
Compare
rebased |
Do you have an example so that I could reproduce? |
Just build any package using hatchling from an extracted sdist inside a gitignored directory, for example: If our top level working git directory is:
With
Then when building a sdist python package from the following build directory which is extracted from the package sdist:
We will end up with an empty wheel file as all paths underneath this are ignored and incorrectly excluded by hatchling:
|
If our project root directory is matched by an exclude pattern we should assume the pattern is invalid as our project root is likely in an ignored build directory of another project.
ed3f60a
to
eafb150
Compare
rebased again |
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 there is an edge case lurking here regarding matching. I feel like rather than checking if the absolute path matches the logic should check the relative path.
In any case I'm going to merge this, thanks!
Hmm, so this may be a pre-existing bug, in buildroot we are in fact currently relying on hatchling not parsing a parent For example hatchling doesn't pick up a
But does pick up a
This is despite to my understanding these having identical meaning by git itself. |
What do you think I should do? |
Probably you would initially want to go through and root cause the reason why hatchling's gitignore logic is not following normal git rules(i.e. not picking up paths like Maybe also try and make sure gitignore path handling is consistent in regards to relative/absolute paths everywhere in the codebase. The relative/absolute path handling in hatchling is at least for me somewhat tricky to reason about due to the complexity of the config.py code. Possibly this complexity is largely unavoidable but I'd assume there would be at least some opportunities there for refactoring in order to simplify things somewhat.
BTW I'm quite confident the general approach I'm using to fixing the bogus parent matching bug in this PR is overall fairly reliable(since it's worked reliably for a while in other projects like poetry), although I'm likewise not entirely sure about all the edge case in regards to absolute/relative paths are being handled correctly here. |
If our project root directory is matched by an exclude pattern we should assume the pattern is invalid as our project root is likely in an ignored build directory of another project.
Should fix: #1641
This is essentially a port of the same technique I used to fix a similar poetry core bug.