Skip to content

Comments

Add cmake_target to change the target used by CMake#477

Merged
henryiii merged 1 commit intoscikit-build:masterfrom
phcerdan:add_cmake_target
Feb 8, 2022
Merged

Add cmake_target to change the target used by CMake#477
henryiii merged 1 commit intoscikit-build:masterfrom
phcerdan:add_cmake_target

Conversation

@phcerdan
Copy link
Contributor

@phcerdan phcerdan commented Apr 9, 2020 •
edited
Loading

Parses the command line for the --target option.
And also provide the cmake_install_target parameter in setup.py

The command line argument has preference over the cmake_install_target from
setup.py.

This allows the user to point to targets other than 'install'.
The user has to set a target in CMake to install only a specific
component.

install(TARGETS foo COMPONENT runtime)
add_custom_target(foo-install-runtime
    ${CMAKE_COMMAND}
    -DCMAKE_INSTALL_COMPONENT=runtime
    -P "${PROJECT_BINARY_DIR}/cmake_install.cmake")

@phcerdan phcerdan force-pushed the add_cmake_target branch 3 times, most recently from 216e1c3 to 5deee00 Compare April 9, 2020 12:14
@jcfr jcfr added the Status: Triage Issues/PRs that need to be triaged label Apr 9, 2020
@jcfr
Copy link
Contributor

jcfr commented Apr 9, 2020

Thanks for the contribution 🙏

I will be able to review tomorrow.

@phcerdan phcerdan force-pushed the add_cmake_target branch 2 times, most recently from a037c7c to dfeaad0 Compare April 9, 2020 13:56
@phcerdan
Copy link
Contributor Author

phcerdan commented Apr 10, 2020

Not sure about the errors in travis, appveyor and azure, they seem unrelated, but let me know if I can help.

@jcfr
Copy link
Contributor

jcfr commented Apr 25, 2020

I will rebase the topic after the fix addressing the Continuous Integration errors is merged. Thanks for your patience 🙏

@jcfr jcfr force-pushed the add_cmake_target branch from dfeaad0 to 8f9b148 Compare April 25, 2020 20:48
jcfr
jcfr requested changes Apr 25, 2020
View reviewed changes
Copy link
Contributor

@jcfr jcfr 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 contribution 🙏

Few nitpicks and it will be good integration.

Make also sure to:

  • update the documentation
  • add an entry to CHANGES file

@phcerdan phcerdan force-pushed the add_cmake_target branch 3 times, most recently from 5d61d89 to b5dacd8 Compare April 27, 2020 21:19
@phcerdan
Copy link
Contributor Author

phcerdan commented Apr 27, 2020 •
edited
Loading

I guess this option is another way to solve #473. Which can also be handled via MANIFEST files from #476.

jcfr
jcfr reviewed Apr 29, 2020
View reviewed changes
Copy link
Contributor

@jcfr jcfr left a comment

Choose a reason for hiding this comment

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

Since I would like to spend some more time testing this, I will not yet integrate it in the upcoming release.

Thanks for your patience

@phcerdan phcerdan force-pushed the add_cmake_target branch 2 times, most recently from 67fd4c9 to 65cb4d0 Compare April 30, 2020 10:10
jcfr
jcfr requested changes Apr 30, 2020
View reviewed changes
@phcerdan
Copy link
Contributor Author

phcerdan commented Apr 30, 2020

Split make function into two: make and make_impl to implement your suggestion:

Then in case cmake_install_target is specified and different from the default value, should we do this:

    cmd = [self.cmake_executable, "--build", source_dir, "--config", config, "--"]
    cmd.extend(clargs)

followed by an other call to

    cmd = [self.cmake_executable, "--build", source_dir, "--target", target, "--config", config, "--"]
    cmd.extend(clargs)

This would ensure the project is always built before building the custom install targets.

@phcerdan
Copy link
Contributor Author

phcerdan commented Apr 30, 2020

Rebase is needed after your review. I splitted it to facilitate review on the make, make_imp refactor.

@phcerdan phcerdan force-pushed the add_cmake_target branch 2 times, most recently from 2b2973f to a8d4328 Compare April 30, 2020 12:20
@phcerdan
Copy link
Contributor Author

phcerdan commented Jun 4, 2020 •
edited
Loading

Let me know if I can help to push this forward @jcfr.
I have been deploying 3 projects to pypi using cmake_install_target from this fork with no problems.
I have rebased the changes on top of master and squashed them in one commit.

@phcerdan phcerdan force-pushed the add_cmake_target branch from a8d4328 to f2ec389 Compare June 4, 2020 09:17
@phcerdan phcerdan requested a review from jcfr June 4, 2020 09:36
@jcfr jcfr mentioned this pull request Jun 10, 2020
Closed
@phcerdan phcerdan mentioned this pull request Jul 27, 2020
Merged
@phcerdan
Copy link
Contributor Author

phcerdan commented Jun 18, 2021

Hi @jcfr, any hope this would be merged?
Github says Changes Requested, but I would say it's a false positive, I think I managed to deal with all the wise points you raised a year ago.

Let me know how/if I can help further. Thanks!

@jcfr
Copy link
Contributor

jcfr commented Jun 18, 2021

Thanks for the contributions, the follow up and for the ping. I plan to spend time working on scikit-build this coming week.

Also, we now have some new core contributors helping out with scikit-build projects, and I am beyond grateful having them join the effort.

@henryiii @mayeut 🙏🔥

@phcerdan
Copy link
Contributor Author

phcerdan commented Jun 21, 2021

@jcfr that's great!

@henryiii
Copy link
Contributor

henryiii commented Jul 4, 2021

Could you rebase (or merge) with master? Otherwise, I can later.

@phcerdan phcerdan force-pushed the add_cmake_target branch from 73fd4d7 to e4a5485 Compare July 4, 2021 20:43
@phcerdan
Copy link
Contributor Author

phcerdan commented Jul 4, 2021

@henryiii rebased on top of current master

@henryiii henryiii closed this Nov 10, 2021
@henryiii henryiii reopened this Nov 10, 2021
@henryiii henryiii closed this Nov 10, 2021
@henryiii henryiii reopened this Nov 10, 2021
@phcerdan @henryiii
Add cmake_target to change the target used by CMake
b8be6ba 
Parses the command line for the `--target` option.
And also provide the `cmake_install_target` parameter in `setup.py`

The command line argument has preference over the `cmake_install_target` from
`setup.py`.

This allows the user to point to targets other than 'install'.
The user has to set a target in CMake to install only a specific
component.

```cmake
install(TARGETS foo COMPONENT runtime)
add_custom_target(foo-install-runtime
    ${CMAKE_COMMAND}
    -DCMAKE_INSTALL_COMPONENT=runtime
    -P "${PROJECT_BINARY_DIR}/cmake_install.cmake")
```

DevNote: There is a refactor of the function `cmaker.make`
That allows calling the function twice in case the target is
not the default `install`.
@henryiii
Copy link
Contributor

henryiii commented Jan 26, 2022

I think we should be careful about expanding the API surface before #601, but this seems safe enough and all review points addressed. @jcfr could you see if this is good and mark the review complete?

@henryiii henryiii mentioned this pull request Jan 31, 2022
Closed
@henryiii henryiii added this to the 0.14.0 milestone Feb 2, 2022
@henryiii henryiii merged commit b76d0d1 into scikit-build:master Feb 8, 2022
@phcerdan phcerdan deleted the add_cmake_target branch February 8, 2022 17:20
@phcerdan phcerdan restored the add_cmake_target branch February 8, 2022 17:21
@phcerdan
Copy link
Contributor Author

phcerdan commented Feb 8, 2022

Thanks @henryiii for taking care of this, please ping me anytime if there is any future issue related with this.

This was referenced Feb 8, 2022
Merged
Merged
@phcerdan phcerdan deleted the add_cmake_target branch April 4, 2022 15:08
@phcerdan phcerdan restored the add_cmake_target branch April 5, 2022 09:59
phcerdan added a commit to phcerdan/SGEXT that referenced this pull request Jan 9, 2023
@phcerdan
Use upstream scikit-build
67e2188 
PR was merged: scikit-build/scikit-build#477
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jcfr jcfr Awaiting requested review from jcfr

Assignees

No one assigned

Labels

Status: Triage Issues/PRs that need to be triaged

Projects

None yet

Milestone

0.14.0

Development

Successfully merging this pull request may close these issues.

3 participants

@phcerdan @jcfr @henryiii