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

Bug/allow rebuilds with scikit build #645

Open
wants to merge 2 commits into
base: branch-24.03
Choose a base branch
from

Conversation

robertmaynard
Copy link

Currently due to the implicit logic around SKBUILD it isn't possible for projects to safely build/install multiple times in a row.

Consider a project that looks like:

legate_add_cpp_subdirectory(cpp TARGET legate_user EXPORT legate_user-export)
legate_default_python_install(legate_user EXPORT legate_user-export)

When a local user runs scikit-build the first time everything will work correctly as legate_user doesn't exist in the install location. But when invoked a second time it will find the installed version, and not build. So to install an updated version
of the files you need to first clean your env which is not a nice pattern.

So with the PR, projects can explicitly state they always want to build from source and never search for a local version.

Avoids errors when `legate_add_cpp_subdirectory` found
a local version and therefore the python targets can't
be added since they already exist
Consider a project that looks like:
```
legate_add_cpp_subdirectory(cpp TARGET legate_user EXPORT legate_user-export)
legate_default_python_install(legate_user EXPORT legate_user-export)
```

If invoked from an env that already has `legate_user` already installed
we can't rebuild from source since we implicitly call `find_package(legate_user)`
due to being in a scikit-build env.
@manopapad manopapad requested review from jjwilke and trxcllnt March 27, 2023 18:29
@trxcllnt trxcllnt added the category:bug-fix PR is a bug fix and will be classified as such in release notes label Mar 27, 2023
@jjwilke
Copy link

jjwilke commented Mar 27, 2023

@robertmaynard
Thanks for catching this!

You mean the find_package below will pick up the installed version instead of the build tree version?

rapids_find_package(${target} CONFIG
      GLOBAL_TARGETS legate::${target}
      BUILD_EXPORT_SET ${LEGATE_OPT_EXPORT}
      INSTALL_EXPORT_SET ${LEGATE_OPT_EXPORT})

I don't think we would ever want that find package to find an installed version. Should I just restrict the find package call to only look in the build tree? I forget what the option is. NO_DEFAULT_PATH or something.

@robertmaynard
Copy link
Author

@robertmaynard Thanks for catching this!

You mean the find_package below will pick up the installed version instead of the build tree version?

rapids_find_package(${target} CONFIG
      GLOBAL_TARGETS legate::${target}
      BUILD_EXPORT_SET ${LEGATE_OPT_EXPORT}
      INSTALL_EXPORT_SET ${LEGATE_OPT_EXPORT})

I don't think we would ever want that find package to find an installed version. Should I just restrict the find package call to only look in the build tree? I forget what the option is. NO_DEFAULT_PATH or something.

Yes you are finding the installed version, yes NO_DEFAULT_PATH would disable those search paths.

I don't recommend naively using the build directory to find the exact package you are building. For example if the user is building and editing the project. After the second execution of CMake they will now find the build directory copy of the project, and won't be able to build it anymore.

@marcinz marcinz changed the base branch from branch-23.05 to branch-23.07 May 18, 2023 20:14
@marcinz marcinz changed the base branch from branch-23.07 to branch-23.09 July 18, 2023 15:39
@marcinz marcinz changed the base branch from branch-23.09 to branch-23.11 September 26, 2023 00:24
@marcinz marcinz changed the base branch from branch-23.11 to branch-24.01 November 9, 2023 16:53
@marcinz marcinz changed the base branch from branch-24.01 to branch-24.03 February 22, 2024 00:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:bug-fix PR is a bug fix and will be classified as such in release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants