search

pantsbuild / pants

The Pants Build System
https://www.pantsbuild.org
Apache License 2.0
3.34k stars 638 forks source link

Allow python protobuf codegen export to coexist with namespace/host package #21665

Closed achrafmam2 closed 1 day ago

achrafmam2 commented 3 days ago

Before this change, if a codegen export clashes with an existing package in the venv the export halts completely. The current change skips the problematic exports. This is sub optimal because not all symbols are exported but better than halting the export.

See #21659

tgolsson commented 3 days ago

Nice! Unfortunately I think the logging here will not be visible, since it would be coming from a child process launched by Pants if I read this code directly -- and thus eaten and not displayed in the common case. I wonder if using shutil.copytree(...., dirs_exist_ok=True) is the simpler solution that solves this permanently?

achrafmam2 commented 3 days ago

Good idea @tgolsson. Done.

tgolsson commented 3 days ago

Perfect! Before I unblock CI, I'll have to ask you to add some release notes as well here... otherwise CI will yell at you anyways :)

https://github.com/pantsbuild/pants/blob/main/docs/notes/2.25.x.md

achrafmam2 commented 3 days ago

@tgolsson thanks for walking me through it. I updated the release notes.

Also note that I updated the code, because when I tested locally it failed. There is an extra step that cleans up the codegen_dir which is unfortunately not tested. Maybe once I have more time I can add more coverage.

Note: I was not going to test this locally because I thought the change is small and what could happen the tests are already passing 😅. I forced myself though and it failed. Maybe it's worth to force new authors to do it.

tdyas commented 2 days ago

I forced myself though and it failed. Maybe it's worth to force new authors to do it.

Thanks for testing! (Even small changes can be larger than they seem ...). (And it means my tests did not hit edge cases of which I was unaware when I wrote this code.)

tgolsson commented 2 days ago

I've been erring on whether we should backport this to 2.24.x and even possibly 2.23.x, what do you think? This was new in 2.22, right?

achrafmam2 commented 1 day ago

Backporting to 2.23 seems good to me. The earlier I can use the fix the better. What should I do beside changing the notes?

tgolsson commented 1 day ago

@achrafmam2 We have a bot that does the PRs etc. Asked in #development on slack, I'm not sure how we do the notes now. The notes update from this branch also shouldn't be backported.

WorkerPants commented 1 day ago

I tried to automatically cherry-pick this change back to each relevant milestone, so that it is available in those older releases of Pants.

:heavy_check_mark: 2.23.x

Successfully opened https://github.com/pantsbuild/pants/pull/21676.

:heavy_check_mark: 2.24.x

Successfully opened https://github.com/pantsbuild/pants/pull/21675.

Thanks again for your contributions!

:robot: Beep Boop here's my run link