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

Python install modifications for compatibility with conda-forge #1737

Merged
merged 13 commits into from Apr 21, 2024

Conversation

bennibbelink
Copy link
Contributor

I think our python installation script is more complex than is necessary and it is causing builds in conda-forge/cyclus to fail at pytests due to not being able to find the cyclus package (stemming from incorrect installation).

My understanding of the issue is as follows:

  • Conda has environment variables to define the installation location (e.g. PREFIX, SP_DIR) to support the portability of our package. These variables are kept as placeholders throughout the build process (you can see this in the install location in the build output - Installing to PREFIX/bin).
  • In the cyclus build we currently generate SetupPyInstall.cmake based on some vars in the build. I think because we configure this during the build (substitute the @@ variables) the placeholder used by conda was not being kept as a placeholder, rather it was expanded out to the absolute path instead of SP_DIR/cyclus (as I saw in build output)
    This PR eliminates the need for configuring a separate CMake script by replacing the SCRIPT argument with CODE.

We can then pass in the conda placeholder via -DPYTHON_SITE_PACKAGES

@bennibbelink bennibbelink marked this pull request as ready for review April 17, 2024 13:43
Copy link

github-actions bot commented Apr 17, 2024

Downstream Build Status Report - ad33d53 - 2024-04-20 13:50:49 -0500

Build FROM cyclus_20.04_apt/cyclus
  • Cycamore: Success
  • Cymetric: Success
Build FROM cyclus_20.04_conda/cyclus
  • Cycamore: Success
  • Cymetric: Success
Build FROM cyclus_22.04_apt/cyclus
  • Cycamore: Success
  • Cymetric: Success
Build FROM cyclus_22.04_conda/cyclus
  • Cycamore: Success
  • Cymetric: Success

@coveralls
Copy link
Collaborator

coveralls commented Apr 17, 2024

Pull Request Test Coverage Report for Build 8767088217

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 2 unchanged lines in 1 file lost coverage.
  • Overall coverage decreased (-0.002%) to 32.599%

Files with Coverage Reduction New Missed Lines %
build/cyclus/py_lib_.cxx 2 34.75%
Totals Coverage Status
Change from base Build 8765205180: -0.002%
Covered Lines: 53079
Relevant Lines: 129079

💛 - Coveralls

@bennibbelink bennibbelink changed the title Python install modifications for compatibility with conda-forge Draft: Python install modifications for compatibility with conda-forge Apr 17, 2024
@bennibbelink bennibbelink changed the title Draft: Python install modifications for compatibility with conda-forge DRAFT: Python install modifications for compatibility with conda-forge Apr 17, 2024
@bennibbelink bennibbelink changed the title DRAFT: Python install modifications for compatibility with conda-forge Python install modifications for compatibility with conda-forge Apr 17, 2024
@bennibbelink bennibbelink marked this pull request as draft April 17, 2024 15:23
@bennibbelink
Copy link
Contributor Author

bennibbelink commented Apr 18, 2024

Ok I had to make one more change (which is really a change to cycamore, but I replicated it here for consistency).

FindCyclus.cmake finds libcyclus.so for cycamore to link against. In the conda cycamore builds the cython generated libraries (eventhooks.so, pyinfile.so, and pymodule.so) were not being linked agains because they were not being reported by FindCyclus.cmake. I will make a PR in cycamore to implement this - I have it working locally with the two respective branches, but in order to see CI pass we will need to merge these changes in cycamore first.

This was not a problem before because we always export LD_LIBRARY_PATH to /root/.local/lib which is the location of these three cython generated libraries. Most literature I can find considers the use of LD_LIBRARY_PATH to be relatively unsafe. With these build changes we no longer need to set LD_LIBRARY_PATH for the linker to find all the libraries it needs.

@bennibbelink
Copy link
Contributor Author

Hmm it looks like making the changes to FindCyclus.cmake in this PR fixed the issue. I now see that Cycamore's CMakeLists.txt we set the CMAKE_MODULE_PATH to look in /root/.local/share/cyclus/cmake which has the Cyclus version of FindCyclus.cmake. Thus modifying this repo's version of the file fixes our issue, but we should make these same changes in cycamore (#596).

@bennibbelink bennibbelink marked this pull request as ready for review April 18, 2024 03:08
@bennibbelink bennibbelink added this to the v1.6 milestone Apr 18, 2024
Copy link
Member

@gonuke gonuke left a comment

Choose a reason for hiding this comment

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

Thanks @bennibbelink - this all looks reasonable. Just one question about installing FindCyclus.cmake "properly"?

cmake/FindCyclus.cmake Show resolved Hide resolved
Copy link
Member

@gonuke gonuke left a comment

Choose a reason for hiding this comment

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

Thanks for the update @bennibbelink

@gonuke gonuke merged commit 5353209 into cyclus:main Apr 21, 2024
10 checks passed
@bennibbelink bennibbelink deleted the pyinstall-cmake branch April 21, 2024 17:38
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.

None yet

3 participants