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

Update cmdclass in setup.py to allow standard installation routine (clean fork) #113

Closed
wants to merge 2 commits into from

Conversation

ajaust
Copy link
Collaborator

@ajaust ajaust commented Jul 22, 2021

This is almost the same as #112, but from a clean fork. I also added a comment to the setup.py about updating the cmdclass object of Versioneer.

Below follows the message in the original PR:
I am currently trying to fix #86 and #87. Instead of trying to use another build phase I tried to go back to the initial setup of a build_ext phase and an install phase when installing the bindings via Spack.

I could actually achieve this by restructuring the setup.py and the defintiion of the cmdclass. I update the internal command classes and this works for me. Together with my Spack package it

  • Creates a _version.py
  • Installs the bindings in the directory specified by Spack

A Spack fork using my fork of the Python bindings can be found here. It contains a new package py-pyprecice-fork for installing that fork.

I am not completely sure of the implications of my changes. Thus, I would need some feedback whether this change of setup.py is legal and whether it breaks any of our tests or use cases.

- This setup.py uses the cmdclass provided by versioneer instead of defining an own one.
- The cmdclass gets updated with our changes. This allows for proper installation of the package while the Spack package can be simplified
Copy link
Member

@BenjaminRodenberg BenjaminRodenberg left a comment

Choose a reason for hiding this comment

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

Looks good to me. It's always a bit difficult to be aware of all the implications. But we have some tests for the most important use-cases and if they work, I think we don't have to worry.

edit: also see my original review here: #112 (review)

I find it confusing that creating a dict and handing it to versioneer.get_cmdclass(my_dict) and your proposed implementation have different outcome. To me both looks pretty much the same. Maybe we should open an issue upstream?

@ajaust
Copy link
Collaborator Author

ajaust commented Jul 27, 2021

I think we can drop this PR for now. I installed [email protected] with the current develop of Spack which contains the changes of spack/spack#25077 and installation worked like a charm. This package does not include the changes I have proposed here anymore.

I assume what I have observed was some side effect of an older Spack version or so. At least I have seen significant higher success rate when installing packages via Spack using the system's Python for the current develop branch.

@BenjaminRodenberg
Copy link
Member

I'll close this PR, since it is not needed anymore. See above. #86 is already closed without the content of this PR. For #87, status is still unclear, but we can reopen this PR, if related to #87.

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.

Phase install_lib fails using Spack if system's Python is used
2 participants