CMake changes to land ahead of releases#5253
Conversation
Also includes a handful of style normalizations that will be distracting while reviewing that PR...
steven-johnson
left a comment
steven-johnson
left a comment
There was a problem hiding this comment.
LGTM with one nit
What's remaining to be done? (Last I looked it was still marked 'draft') |
I've got it working for CMake, but haven't touched the Makefiles. |
tools/package-unix.zsh
Outdated
| @@ -5,13 +5,15 @@ DIR="$(cd "$(dirname "${(%):-%N}")"/.. >/dev/null 2>&1 && pwd)" | |||
| [ -z "$LLVM_DIR" ] && echo "Must set specific LLVM_DIR for packaging" && exit | |||
There was a problem hiding this comment.
...It's really weird to have both package-unix.zsh and package-unix.sh.
There was a problem hiding this comment.
(to be more specific: it would be vastly preferable to have a single .sh that will work under both zsh and bash.)
There was a problem hiding this comment.
This works on my mac for both bash and zsh:
DIR=$(cd -P -- "$(dirname -- "$0")" && printf '%s\n' "$(pwd -P)/")
There was a problem hiding this comment.
The #!/bin/bash up top renders that moot. But I guess Apple hasn't deleted bash from their OS yet. We can just drop the zsh script.
There was a problem hiding this comment.
In that case, we should probably nuke Unix.cmake too and just toll it into the unix shell script. (Sorry for all the change requests after approval...)
|
Suggestion: also change |
Done |
steven-johnson
left a comment
steven-johnson
left a comment
There was a problem hiding this comment.
Yet another suggestion for the packaging scripts: it's probably preferable to have the buildbots use these scripts directly (rather than replicate the logic) so that there's a single point of truth; however, these need to be a bit more configurable for that to work well. In particular, being able to explicitly specify the source and build dirs (rather than inferring them relative to the script) is really gonna be necessary.
tools/package-unix.zsh
Outdated
| @@ -5,13 +5,15 @@ DIR="$(cd "$(dirname "${(%):-%N}")"/.. >/dev/null 2>&1 && pwd)" | |||
| [ -z "$LLVM_DIR" ] && echo "Must set specific LLVM_DIR for packaging" && exit | |||
There was a problem hiding this comment.
This works on my mac for both bash and zsh:
DIR=$(cd -P -- "$(dirname -- "$0")" && printf '%s\n' "$(pwd -P)/")
If I could snap my fingers and make it happen, the buildbots would read all of their config out of the repository. The |
|
The failure for x86-64-linux-testbranch-trunk-cmake is unrelated and should not block landing |
|
Sounds good. Is there anything else you need in the package scripts? |
The scripts look good for now. Ready to land once the buildbots get to 18 run. |
|
ok, I think this has enough test coverage to land. |
2 participants
I would still love to get #5228 merged before the CppCon talk so that people can use the autoschedulers, but these changes at least will let us update the buildbots now.
This PR consolidates all the CMake packaging logic into
packaging/CMakeLists.txt. It had been spread around a bit, but this was getting harder to justify.It also makes the packaging scripts which invoke the build multiple times suitable for buildbot use.
Fixes #4738