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

Provide package_dir for setting subdirectory of project [alternative] #319

Merged
merged 15 commits into from
Apr 14, 2020

Conversation

joerick
Copy link
Contributor

@joerick joerick commented Apr 10, 2020

I hope you don't mind @filips123 - I've taken this further. It's turned out to be quite a fundamental change! So maybe easier for me to do it.

Still to go are tests, but the comments in the previous PR have been addressed.

@joerick
Copy link
Contributor Author

joerick commented Apr 10, 2020

Tests are now in! Ready for review.

@YannickJadoul
Copy link
Member

Will review, but I'll take my time for a thorough comparison, this weekend.

Copy link
Contributor

@Czaki Czaki left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Short review, Looks good for me. But maybe move this test to be one of first and then use this ability to reduce number of spam.c files? (in other PR)

test/13_subdir_package/bin/before_build.py Show resolved Hide resolved
@joerick
Copy link
Contributor Author

joerick commented Apr 11, 2020

Thanks for the review!

But maybe move this test to be one of first and then use this ability to reduce number of spam.c files? (in other PR)

I'm not sure what you mean... are you referring to #277 ?

@Czaki
Copy link
Contributor

Czaki commented Apr 11, 2020

yes. but I thin that for many test it can be used one sample package so maybe instead of of templates and dynamic write of files use this PR result.

Copy link
Member

@YannickJadoul YannickJadoul left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be fair, I don't see a lot of differences with #295. Maybe some extra cleaning up and the added test, or am I missing something fundamental?

So yeah, I don't have a lot of remarks. Looks pretty good to me :-)
Maybe an extra warning in the docs might be good to specify that now the whole pwd is copied when running Linux on this (which is potentially costly)?

cibuildwheel/linux.py Show resolved Hide resolved
cibuildwheel/macos.py Show resolved Hide resolved
docs/options.md Show resolved Hide resolved
test/13_subdir_package/bin/before_build.py Show resolved Hide resolved
@YannickJadoul
Copy link
Member

I'm not sure what you mean... are you referring to #277 ?

I would keep that separate, btw. We should try to solve this, but the difference between 12 test folders before this PR and 13 folders after isn't going to make a difference here. I do agree with the thing that somehow tests are less and less in a "logical" order (but that's also another discussion).

@joerick
Copy link
Contributor Author

joerick commented Apr 12, 2020

To be fair, I don't see a lot of differences with #295. Maybe some extra cleaning up and the added test, or am I missing something fundamental?

No, that's all it is - I just took @filips123's PR and fixed a couple things.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants