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 python setup.py build sdist #84

Closed
thewtex opened this issue Jul 18, 2016 · 10 comments
Closed

Fix python setup.py build sdist #84

thewtex opened this issue Jul 18, 2016 · 10 comments
Assignees
Labels
Type: Bug Something's not working correctly.
Milestone

Comments

@thewtex
Copy link
Member

thewtex commented Jul 18, 2016

Currently this creates a tarbal with the contents of _skbuild/cmake-install.
Instead, it should contain the project source code so that the sdist can be
downloaded and a platform-specific binary wheel can be created from it.

@jcfr jcfr added the Type: Bug Something's not working correctly. label Jul 18, 2016
@opadron
Copy link
Member

opadron commented Jul 19, 2016

I'm taking a look at this now, and the way to handle this is not trivial. Distutils and setuptools expect to have information about extension modules and their sources. Because we're preempting their build logic, we're effectively removing the information that they need to correctly produce a source package.

We would need to provide our own mechanism for identifying the files that should be included in an sdist tarball. One possibility is to include (yet another) CMake module with a function that you would call on each of your source files. Then, we can configure the build and inspect the output of that function without having to actually build anything.

I'm open to other ideas.

@blowekamp
Copy link
Contributor

Currently SimpleITK has a script files it uses to create tar ball [1].

The important thing to determine is what is needed to enable pip to be able to download, build and install from a source or tarball.

[1] https://github.com/SimpleITK/SimpleITK/blob/master/Utilities/Maintenance/SourceTarball.bash

@opadron
Copy link
Member

opadron commented Jul 19, 2016

Basically, we need packages that use skbuild to provide their own version of "install_manifest.txt", but for their sources. It could be a created by the CMake project, a package-specific script, or just a flat text file.

@blowekamp
Copy link
Contributor

Do you have an example of this structure where the "install_manifest.txt" is used for the source tarball?

From scikit-build
python setup.py sdist && tar -ztvf ./dist/scikit-build-0.2.0.tar.gz |grep install

I don't see install_manifest.txt in sdist?

@opadron
Copy link
Member

opadron commented Jul 19, 2016

Right, I forgot distutils was already doing this!

So, it looks like the best solution is to simply not change the paths to the various packages declared in the setup() call, and require folks to provide their own MANIFEST(.in).

If so, this would be much less work than I had originally feared! :)

@thewtex
Copy link
Member Author

thewtex commented Jul 19, 2016

Could we generate a sensible default if not explicitly present? If .git/ is present, then the output of git archive would be a good default. If .git/ is not present, then all sources excluding dist/ and _skbuild would be a good default.

If those defaults do not work for projects, then they could write a MANIFEST.in file...

@opadron
Copy link
Member

opadron commented Jul 19, 2016

@thewtex yes, that sounds reasonable to me.

@jcfr jcfr closed this as completed in a1ea78f Jul 22, 2016
jcfr added a commit that referenced this issue Jul 22, 2016
* setuptools-topic-reorganized:
  Refactor tests introducing reusable decorators. See #63, #78
  style: Rename distutils_wrap to setuptools_wrap
  Introduce bdist and bdist_wheel command. Fixes #84, Fixes #85
  Add setuptools dependency. Fixes #68
  cmaker: Reintroduce exceptions when using sysconfig
  style: Simplify distutils_wrap removing intermediate var
  style: Fix indent in cmaker
jcfr added a commit that referenced this issue Jul 22, 2016
* setuptools-topic-reorganized:
  Refactor tests introducing reusable decorators. See #63, #78
  style: Rename distutils_wrap to setuptools_wrap
  Introduce bdist and bdist_wheel command. Fixes #84, Fixes #85
  Add setuptools dependency. Fixes #68
  cmaker: Reintroduce exceptions when using sysconfig
  style: Simplify distutils_wrap removing intermediate var
  style: Fix indent in cmaker
@thewtex thewtex reopened this Jul 22, 2016
@thewtex
Copy link
Member Author

thewtex commented Jul 22, 2016

Checking current master, this issue has yet to be addressed.

@opadron
Copy link
Member

opadron commented Jul 22, 2016

@thewtex That's right. sdist handling is still on the todo list.

But, building wheels from a manual checkout should work.

@jcfr jcfr added this to the 0.2.0 milestone Jul 25, 2016
@jcfr jcfr modified the milestones: 0.5.0, 0.4.0 Aug 19, 2016
@jcfr jcfr modified the milestones: 0.4.0, 0.5.0 Aug 31, 2016
@jcfr jcfr closed this as completed in 16a8789 Sep 1, 2016
jcfr added a commit that referenced this issue Sep 1, 2016
Implement source distributions. fixes #84
jcfr added a commit that referenced this issue Sep 2, 2016
Following 16a8789 (add (terrible) hack for implementing source
distributions. fixes #84), MANIFEST file is generated in samples
directories.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Bug Something's not working correctly.
Projects
None yet
Development

No branches or pull requests

4 participants