Skip to content

Minor build script enhancements #349

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

Merged
merged 1 commit into from
Oct 9, 2024

Conversation

AndreyPavlenko
Copy link
Contributor

@AndreyPavlenko AndreyPavlenko commented Sep 19, 2024

  • Added -s option for the build dir suffix.
  • Added -v option that allows to build LLVM only.
  • Get the IMEX repository url directly from the cmake file.
  • Changed the IMEX source directory to imex-src - this directory is also used by cmake.

@AndreyPavlenko AndreyPavlenko marked this pull request as ready for review September 19, 2024 22:38
@dchigarev
Copy link
Contributor

@leshikus are you okay with the changes?

Copy link
Contributor

@leshikus leshikus left a comment

Choose a reason for hiding this comment

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

Sorry, forgot to finalize

eval "$ASSIGN_NEXT=$arg"
unset ASSIGN_NEXT
continue
fi
Copy link
Contributor

Choose a reason for hiding this comment

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

there is better, more transparent and standard way to use this; while [ -v 1 ]; do ; -v is bash specific, one may use -n $1 instead

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But you have to use shift in each case statement. How is it better? For single value arguments (that we had before) for arg in "$@" seems much less verbose.

fi

[ -z "$CLEANUP" ] || rm -rf build
cmake -S . -G Ninja -B build \
BUILD_DIR="$PROJECT_DIR/build$BUILD_DIR_SFX"
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe adding dir suffixes is overengineering as well as the previous attempt to make LLVM location configurable; the common approach is the following: each build environment has exactly the same directories; if you need another environment, you just clone the existing as a whole, this can be done via docker or rsync command; this makes scripts less confusing

Copy link
Contributor Author

Choose a reason for hiding this comment

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

With suffixes I can have different builds (debug, release, with imex, without imex) without changing my dev environment. In this way, I can test my changes in different configurations. When creating clones, I also need to populate my changes to each clone, having IDE configurations for each clone, etc.

Copy link
Contributor

Choose a reason for hiding this comment

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

I guessed the intention correctly. My point is the compile script is incorrect tool to implement that logic. You've already have a number of existing tools like docker. We can think together about separate clone_env script, it may use docker or rsync or and git checkout of some dev branch.

Why each environment should look as another? This follows standard linux approach "clone the project to directory and run make test". People expect this, and they never learn your suffixes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suffixes are optional, people are not required to use them. A simple use-case - I've made some changes (not yet committed) in my IDE and want to test with and without IMEX. For this I need not only to build the project with different options, but also to build 2 different versions of LLVM. I just launch the build script from my build env with different suffixes.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is it possible to have these changes as a part of a separate script?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is definitely possible, the main question - why do we need one more script?

Copy link
Contributor

Choose a reason for hiding this comment

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

everyone uses compile.sh; how many users of compile.sh will be using these suffixes? likely only you; that's separate things we put into compile.sh from the ones we put into some other script

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But creating a separate script just for me does not make sense either.

@AndreyPavlenko AndreyPavlenko merged commit 37a70e6 into intel:main Oct 9, 2024
6 checks passed
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.

4 participants