Provide `package_dir` for setting subdirectory of project #295

filips123 · 2020-03-08T16:15:23Z

This PR creates a way to provide package_dir for setting subdirectory of project and closes #294.

I added additional package_dir argument, which should be a subdirectory of project_dir (written as a normal relative path to cwd), which should contain the Python package to build. That argument is then passed to pip wheel command to build the wheels.

For example, GitHub project contains some C++ code, while Python bindings are located in bindings/python. Then project_dir should be set to ., while package_dir should be set to bindings/python. So when building wheels, only bindings/python would be used, while whole project_dir would be copied to Docker container.

@joerick Can you check this?

Czaki · 2020-03-08T18:58:33Z

I do not see any tests in this PR that check if this solution works.

filips123 · 2020-03-08T19:57:01Z

I will create tests tomorrow.

joerick · 2020-03-09T09:17:54Z

Hi @filips123 ! Thanks for putting this together!

In addition to the tests that Czaki mentioned above, (and maybe before that), I have an API request for this - would it be possible to change the arguments slightly?

This PR has made me realise some inconsistencies in how relative paths are handled in cibuildwheel, currently. If a user writes CIBW_BEFORE_BUILD=bash ./scripts/prebuild.sh, then on Linux that path is resolved relative to project_dir (since it's copied in to /project), but on Mac/Windows, those paths are relative to the working directory, because we don't cd as part of cibuildwheel's operation.

This hints at two things:

Probably nobody uses the project_dir parameter for anything other than ., since I haven't seen a bug report for it
This PR could exploit the above to neaten the interface of this feature.

Of the two behaviours, I think the Mac/Windows one makes the most sense - the CWD of the options is the same as how cibuildwheel was invoked.

Currently you have:

env:
  CIBW_BEFORE_BUILD: bash ./scripts/prebuild.sh
script:
  - cibuildwheel --package_dir ./bindings/python .

It's kinda ambiguous how ./scripts/prebuild.sh would resolve above. I'd propose that we make cibuildwheel treat the working directory as the project directory, and change the positional argument to be package_dir:

env:
  CIBW_BEFORE_BUILD: bash ./bindings/python/scripts/prebuild.sh
script:
  - cibuildwheel ./bindings/python

I think this is better, since we can have clear and consistent rules about how options like CIBW_BEFORE_BUILD and CIBW_TEST_COMMAND resolve their relative paths, which would 99% of the time be the root of the git repo.

Having {package} available in those options would be really handy, too.

env:
  CIBW_BEFORE_BUILD: bash {package}/scripts/prebuild.sh
script:
  - cibuildwheel ./bindings/python

Sorry to complicate things! But it's important to get the API right before user's config crystallises around it.

YannickJadoul · 2020-03-09T14:55:01Z

I quite like @joerick's idea; I'm a little bit hesitant towards having too many different path/folder arguments/options to configure this.

As mentioned in #294, another option is getting to the host filesystem through /host, but that's not supported on CircleCI. So an alternative to this PR would be to somehow provide a better way of accessing/copying stuff from outside the project directory into the docker image?

filips123 · 2020-03-13T11:47:40Z

@joerick So baisically I can just rename project_dir to package_dir, provide {package} to build scripts and use it to build wheels in that directory?

joerick · 2020-03-13T11:56:20Z

Yes. And make sure that linux.py copies the working dir into the container, rather than the package_dir

filips123 · 2020-03-13T12:50:56Z

I updated code to use . as project_dir, path provided as argument as package_dir and provided {package} to build scripts. It should now copy project_dir (so current directory) to Docker image, but build wheel from package_dir (provided by user).

I will update tests a bit later.

YannickJadoul · 2020-03-29T17:34:13Z

cibuildwheel/__main__.py

        exit(2)

    if args.print_build_identifiers:
        print_build_identifiers(platform, build_selector)
        exit(0)

    build_options = dict(
-        project_dir=project_dir,
+        project_dir='.',


What about calling os.getcwd() here?

Or even better: killing the project_dir parameter, and just calling os.getcwd() in linux.py/macos.py/windows.py ?

I left project_dir because it still allows a bit advanced configuration if needed. But I can update it to os.getcwd().

YannickJadoul · 2020-03-29T17:36:13Z

cibuildwheel/linux.py

            test_requires=' '.join(test_requires),
            test_extras=test_extras,
            test_command=shlex.quote(
-                prepare_command(test_command, project='/project') if test_command else ''
+                prepare_command(test_command, project=container_project_dir, package=container_package_dir) if test_command else ''


Why the useless/constant container_project_dir variable?

It is also used in various places in this file, so I thought it would be better to store it in one place.

YannickJadoul · 2020-03-29T17:38:05Z

cibuildwheel/macos.py

@@ -175,14 +176,14 @@ def build(project_dir, output_dir, test_command, before_test, test_requires, tes

        # run the before_build command
        if before_build:
-            before_build_prepared = prepare_command(before_build, project=abs_project_dir)
+            before_build_prepared = prepare_command(before_build, project=abs_project_dir, package=abs_package_dir)


project=abs_project_dir is also not necessary anymore, then, since that should just be the outcome of running pwd in the CIBW_BEFORE_BUILD command.

It would be necessary to keep project for backward compatibility, I think, since it might appear in user configs (similarly to how we expand {python} to python in some options). We could deprecate and remove from the docs, though.

Yup, I like that idea! :-)

So here I just need to update docs to remove it?

YannickJadoul · 2020-03-29T17:45:41Z

A few things, here:

We should still have a few tests (or maybe extend old tests) to check for differences between project and package directories (i.e., is the root of the project directory that's copied indeed equal to the pwd of the test?)
I'm a bit worried about users that would before call cibuildwheel project/. This should still work, but will now potentially copy a lot more things into the docker container, if I understand correctly? Won't that take a lot of time? At the very least, we need a big warning in the README/docs, stating that it now matters from where you call cibuildwheel.
Finally, if the project directory is implicit anyway, then I think all references to project and project_dir should be removed from the user. So replacing {project} etc. should at least be deprecated, if not just disabled. If users are relying on {project}, a failing build might be nice, such that the breaking changes are checked?

filips123 · 2020-03-29T18:30:50Z

I'm a bit worried about users that would before call cibuildwheel project/. This should still work, but will now potentially copy a lot more things into the docker container, if I understand correctly?

I originally made that both project_dir and new package_dir are exposed as arguments, but I later removed it as suggested by @joerick:

Probably nobody uses the project_dir parameter for anything other than ., since I haven't seen a bug report for it.
This PR could exploit the above to neaten the interface of this feature.

YannickJadoul · 2020-03-29T18:36:48Z

I originally made that both project_dir and new package_dir are exposed as arguments, but I later removed it as suggested by @joerick:

I know, yes, and I liked the idea as well, back then. It wasn't meant as critique, but it's something we might want to still think about? Maybe we just need to print a warning, or so, when the argument is set? Or is there another way to more safely make the transition?

joerick

I'm a bit worried about users that would before call cibuildwheel project/. This should still work, but will now potentially copy a lot more things into the docker container, if I understand correctly?

Yes. It'll still work, but it might be slow. My strong suspicion is that very few people are calling cibuildwheel with a path, for the reasons noted above. I'll make it clear in the release notes what the changes are, and we'll need to call this out in the docs somewhere. Maybe a note in the package_dir helpstring, linking to an entry in Tips and Tricks? Or perhaps in the Options section. Happy to help out with this @filips123 if you'd like?

joerick · 2020-03-29T19:10:28Z

cibuildwheel/linux.py

+    abs_package_dir = os.path.abspath(package_dir)
+
+    container_project_dir = '/project'
+    container_package_dir = os.path.join(container_project_dir, os.path.relpath(abs_package_dir, os.path.commonprefix([abs_project_dir, abs_package_dir]))),


Would container_package_dir = os.path.join('/project', package_dir) do the same here?

Actually, it should be possible to pass package_dir to the docker container directly, without any path operations in Python, I think.

Not if package_dir was passed as absolute path, I guess? Or, in that case we'd need to specify it needs to be a relative path?

ah yeah, good point. Then the relpath step would still be needed, but it could be left relative to the working dir and used like that inside the container.

joerick · 2020-04-08T11:02:13Z

Would you mind if I helped out with this @filips123 ? I've got a bit of time spare and would like to get this merged before we go ahead with some project-wide refactoring.

filips123 · 2020-04-08T12:13:08Z

I didn't have much time in last few days. I will try to resolve some of the requested changes. But I will probably need some help with changing docs to provide information about project_dir and package_dir.

joerick · 2020-04-09T13:17:00Z

There have been a lot of changes lately so I've merged master for you. Hope this didn't duplicate work!

joerick · 2020-04-10T12:01:44Z

I've taken a crack at this myself - see #319.

filips123 added 2 commits March 13, 2020 13:45

Use package_dir to specify path to Python package

1e08d29

Update docs

bbb77aa

filips123 force-pushed the support-subdirectory branch from 2702767 to bbb77aa Compare March 13, 2020 12:47

Fix tests

ed50cfd

YannickJadoul reviewed Mar 29, 2020

View reviewed changes

joerick reviewed Mar 29, 2020

View reviewed changes

joerick mentioned this pull request Apr 8, 2020

Add type annotations and drop Python 3.5 host support #315

Merged

Merge remote-tracking branch 'origin/master' into support-subdirectory

d5010b8

joerick mentioned this pull request Apr 12, 2020

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

Merged

joerick merged commit d5010b8 into pypa:master Apr 14, 2020

YannickJadoul mentioned this pull request Dec 19, 2020

[Feature Request] Optionally build wheels with bdist_wheel instead of pip #486

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Provide `package_dir` for setting subdirectory of project #295

Provide `package_dir` for setting subdirectory of project #295

filips123 commented Mar 8, 2020

Czaki commented Mar 8, 2020

filips123 commented Mar 8, 2020

joerick commented Mar 9, 2020

YannickJadoul commented Mar 9, 2020

filips123 commented Mar 13, 2020

joerick commented Mar 13, 2020

filips123 commented Mar 13, 2020

YannickJadoul Mar 29, 2020

YannickJadoul Mar 29, 2020

filips123 Apr 8, 2020

YannickJadoul Mar 29, 2020

filips123 Apr 8, 2020

YannickJadoul Mar 29, 2020

joerick Mar 29, 2020 •

edited

Loading

YannickJadoul Apr 5, 2020

filips123 Apr 8, 2020

YannickJadoul commented Mar 29, 2020

filips123 commented Mar 29, 2020

YannickJadoul commented Mar 29, 2020

joerick left a comment

joerick Mar 29, 2020

joerick Mar 29, 2020

YannickJadoul Mar 29, 2020

joerick Apr 5, 2020

joerick commented Apr 8, 2020

filips123 commented Apr 8, 2020

joerick commented Apr 9, 2020

joerick commented Apr 10, 2020

Provide package_dir for setting subdirectory of project #295

Provide package_dir for setting subdirectory of project #295

Conversation

filips123 commented Mar 8, 2020

Czaki commented Mar 8, 2020

filips123 commented Mar 8, 2020

joerick commented Mar 9, 2020

YannickJadoul commented Mar 9, 2020

filips123 commented Mar 13, 2020

joerick commented Mar 13, 2020

filips123 commented Mar 13, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joerick Mar 29, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

YannickJadoul commented Mar 29, 2020

filips123 commented Mar 29, 2020

YannickJadoul commented Mar 29, 2020

joerick left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joerick commented Apr 8, 2020

filips123 commented Apr 8, 2020

joerick commented Apr 9, 2020

joerick commented Apr 10, 2020

Provide `package_dir` for setting subdirectory of project #295

Provide `package_dir` for setting subdirectory of project #295

joerick Mar 29, 2020 •

edited

Loading