Skip to content
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

fix PEP420 namespace packages #1090

Merged
merged 2 commits into from
Jul 5, 2016
Merged

fix PEP420 namespace packages #1090

merged 2 commits into from
Jul 5, 2016

Conversation

ivan-kalev
Copy link
Contributor

See #1074 for details.

@msarahan
Copy link
Contributor

msarahan commented Jul 5, 2016

@Ikalev sorry I forgot earlier, this has been proposed once before: #859

There, I suggested one change: throw an exception if copying would overwrite any file(s). I think that this is very important for sanity - otherwise it is ill-defined what the contents of your files are. Any ideas on that point?

@ivan-kalev
Copy link
Contributor Author

@msarahan It's a bit of a grey zone: If we are merging namespace packages some overwriting might happen and that might be okay in edge cases... But otherwise it would be an error indeed.

I think it's reasonable to ignore said edge cases and assume that this is an error. I'll try to extend the implementation with an explicit check. Should be easy.

@ivan-kalev
Copy link
Contributor Author

@msarahan should be good to merge now.

@msarahan
Copy link
Contributor

msarahan commented Jul 5, 2016

Looks great. Minor PEP8 errors and existing errors on Travis. Will wait for AppVeyor, then merge.

@msarahan
Copy link
Contributor

msarahan commented Jul 5, 2016

(I am happy to fix PEP8 - you don't need another push.)

@msarahan
Copy link
Contributor

msarahan commented Jul 5, 2016

Thanks again, @Ikalev

@ivan-kalev
Copy link
Contributor Author

Great, thanks!

@183amir
Copy link
Contributor

183amir commented Jul 5, 2016

Thank you this is awesome. Sorry I was busy and could not find a time to write proper test cases for my pull request.

@ivan-kalev
Copy link
Contributor Author

Note that this does not solve all cases of #649 . Python 2 namespace packages (declared with setuptools or distutils) contain __init__.py files, which copy_into() might attempt to overwrite. This will obviously not pass our test for merge conflicts. The workaround on Python 2 is to use pkg_resources. declare_namespace() and enable preserve_egg_dir, which hides shared namespace files in private egg folders.

@ivan-kalev
Copy link
Contributor Author

@msarahan I don't see these changes in the source tree at the moment. I deleted my fork after the merge, but this should be fine, right? I'm confused.

@msarahan
Copy link
Contributor

msarahan commented Jul 8, 2016

@Ikalev could you please resubmit this? I don't know how, but I don't see it in the git history. Nor do I see any force pushes that I would have overwritten it with...

@ivan-kalev
Copy link
Contributor Author

@msarahan Unfortunately I have deleted my fork of the repo, but I can try to extract a patch from this diff and prepare a new PR: https://github.com/conda/conda-build/pull/1090/files

@msarahan
Copy link
Contributor

msarahan commented Jul 8, 2016

Sorry for the mess. I can submit this PR for you, if you'd prefer.

@ivan-kalev
Copy link
Contributor Author

No problem. Here is the new PR: #1105.

@github-actions github-actions bot added the locked [bot] locked due to inactivity label May 14, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 14, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
locked [bot] locked due to inactivity
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants