Local semver#681
Conversation
mcarans
left a comment
There was a problem hiding this comment.
Thanks for looking into this. It's a step in the right direction. See my comments for some requested changes
| @@ -0,0 +1,109 @@ | |||
| #!/bin/bash | |||
There was a problem hiding this comment.
It's great you made a helper script for this. It could probably be put in ShellScripts/common as it looks like it could work on Windows MSYS2 as well.
This should be made a bash function to avoid spawning a subshell. An example is this existing function: https://github.com/OoliteProject/oolite/blob/master/ShellScripts/Linux/dep_location_fn.sh#L3 which is then called like this: https://github.com/OoliteProject/oolite/blob/master/ShellScripts/Linux/build_gnustep.sh#L11-L12.
I add _fn to the script name ie. download_github_fn.sh to distinguish that it is a function to be called by other scripts.
If you want to also be able to call it like a script for your own use in bash, then you can make a separate download_github.sh that sources the function from download_github_fn.sh (but the install_packages_root.sh script should use the function directly to avoid making a subshell).
| @@ -13,7 +13,7 @@ jobs: | |||
| runs-on: ubuntu-latest | |||
There was a problem hiding this comment.
With gitversion called in the build itself in get_version.sh, why is the dependency on common still needed? We need to get rid of the dependency on common so that each Windows and Linux build is self contained otherwise it's hard to replicate build failures that occur in CI locally. If it is informational, then it can run without being a dependency (remove the needs: [common] lines) in which case it should be renamed to something like ci_information.
| { | ||
| #ifdef BUILD_DATE | ||
| NSString *caption = [NSString stringWithFormat:@"Oolite v%@ - %s", @OO_VERSION_FULL, BUILD_DATE]; | ||
| NSString *caption = [NSString stringWithFormat:@"Oolite v%@ by %@ - %s", @OO_VERSION_FULL, @OO_BUILDER, BUILD_DATE]; |
There was a problem hiding this comment.
If OoliteProject/oolite is forked and a package is built by CI for the fork, what will OO_BUILDER be? It shouldn't be OoliteProject or whatever name is associated with the main repo.
| pushd "$SCRIPT_DIR" | ||
|
|
||
| # install gitversion | ||
| GITVERSION_TGZ=$(./download_github.sh --owner GitTools --repository GitVersion --filter linux-x --outputdir ${SCRIPT_DIR}) |
There was a problem hiding this comment.
$() creates a subshell - source download_github_fn.sh and call a function in there called download_github instead
Fixes #676 by introducing semver to local builds and adding the BUILDER environment variable.