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

Make python component part of build, not install #524

Merged
merged 3 commits into from
Sep 27, 2023

Conversation

AlexanderRichert-NOAA
Copy link
Contributor

@AlexanderRichert-NOAA AlexanderRichert-NOAA commented Sep 26, 2023

Proposing a solution for #520, which is to make the python module build part of the build step, not the install step. This PR rearranges python/CMakeLists.txt to use a new target called "python_mod", with which a custom build command can be run to take care of the setup.py invocation. With these changes, setup.py "installs" into the build space, then that directory is copied to the appropriate location by "make install". I took out the first of the two "python setup.py build" invocations because it didn't seem to be necessary but I can reinstate if needed.

@AlexanderRichert-NOAA AlexanderRichert-NOAA marked this pull request as ready for review September 26, 2023 04:26
@climbfuji
Copy link
Contributor

I am removing myself from the list of reviewers (but appreciate the heads up), because I don't really know enough about the NCEPLIBS-bufr code.

@climbfuji
Copy link
Contributor

I am removing myself from the list of reviewers (but appreciate the heads up), because I don't really know enough about the NCEPLIBS-bufr code.

Well, I would if I could ;-)

Copy link
Contributor

@aerorahul aerorahul left a comment

Choose a reason for hiding this comment

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

I think this is much cleaner and more elegant than the previous solution.
Thanks for the update @AlexanderRichert-NOAA

Copy link
Contributor

@aerorahul aerorahul left a comment

Choose a reason for hiding this comment

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

Apologies. I meant to approve, but had the button on "Request changes"

@aerorahul aerorahul removed the request for review from climbfuji September 27, 2023 15:54
@aerorahul
Copy link
Contributor

I am removing myself from the list of reviewers (but appreciate the heads up), because I don't really know enough about the NCEPLIBS-bufr code.

Well, I would if I could ;-)

boo. there is nothing to know about the bufr code here. its just regular cmake and installation stuff.

@climbfuji
Copy link
Contributor

I am removing myself from the list of reviewers (but appreciate the heads up), because I don't really know enough about the NCEPLIBS-bufr code.

Well, I would if I could ;-)

boo. there is nothing to know about the bufr code here. its just regular cmake and installation stuff.

Just needed an excuse for not having the time ...

@jbathegit jbathegit merged commit 2672f7e into NOAA-EMC:develop Sep 27, 2023
6 checks passed
@jbathegit
Copy link
Collaborator

Thanks Alex and Rahul!

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