This repository has been archived by the owner on Nov 25, 2024. It is now read-only.
Refactoring and fixing build #22
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
BradReesWork
added
improvement
dongxuy04
reviewed
Jun 1, 2023
Co-authored-by: dongxuy04 <[email protected]>
|
@dongxuy04 thanks for catching that cut&paste error |
rlratzel
suggested changes
Jun 5, 2023
build.sh
Outdated
| --compile-cmd | ||
| --clean | ||
| -h | ||
| --help" |
There was a problem hiding this comment.
I think the closing quote needs to be on a new line by itself (see feedback below)
build.sh
Outdated
| @@ -94,29 +140,62 @@ if (( ${NUMARGS} != 0 )); then | |||
| # Check for cmake args | |||
| cmakeArgs | |||
| for a in ${ARGS}; do | |||
| if ! (echo " ${VALIDARGS} " | grep -q " ${a} "); then | |||
| if ! (echo " ${VALIDARGS} " | grep -q "^[[:blank:]]*${a}$"); then | |||
There was a problem hiding this comment.
There's a slight bug with the change above where the values for VALIDARGS are one-per-line. I tried this locally and the last arg in VALIDARGS is no longer recognized:
~$ ./build.sh --help
Invalid option: --help
I think this just needs to have the closing quote for --help where VALIDARGS is set above be on a separate line. Using the new grep pattern, you can also drop the spaces around ${VALIDARGS}:
Suggested change
| if ! (echo " ${VALIDARGS} " | grep -q "^[[:blank:]]*${a}$"); then | |
| if ! (echo "${VALIDARGS}" | grep -q "^[[:blank:]]*${a}$"); then |
build.sh
Outdated
| echo "Invalid option: ${a}" | ||
| exit 1 | ||
| fi | ||
| echo " args are ${a} " |
build.sh
Outdated
| # setup.py and cmake reference an env var LIBWHOLEGRAPH_DIR to find the | ||
| # libwholegraph package (cmake). | ||
| # If not set by the user, set it to LIBWHOLEGRAPH_BUILD_DIR | ||
| LIBWHOLEGRAPH_DIR=${LIBWHOLEGRAPH_DIR:=${LIBWHOLEGRAPH_BUILD_DIR}} | ||
| if ! hasArg --compile-cmd; then | ||
| cd ${REPODIR}/pylibwholegraph | ||
| cd ${REPODIR}/python/pylibwholegraph | ||
| echo "Changed to: ${PWD} and ${PYTHON}" |
|
LGTM, thanks! |
dongxuy04
approved these changes
Jun 6, 2023
rlratzel
approved these changes
Jun 9, 2023
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Do not be scared by the large number of files. I moved directories and it includes all the files in those directories.
Only files changed
Moved some folders around to better follow RAPIDS.
Changes some build commons to match other RAPIDS so that arguments are consistent
updated the build process (cmake) for the new paths/folders
Added an empty CHANGELOG file
The next PR will address docs, so ignore that part of the build script