-
Notifications
You must be signed in to change notification settings - Fork 17
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
Conversation
61d1e5c
to
33bc7a2
Compare
@leshikus are you okay with the changes? |
There was a problem hiding this 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 |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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" |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
-s
option for the build dir suffix.-v
option that allows to build LLVM only.imex-src
- this directory is also used by cmake.