On Mon, 29 May 2023 13:21:27 GMT, Erik Helin <[email protected]> wrote:
> Hi all, > > please review this smaller patch to the build system. This patch changes what > the build system will do when there are multiple configurations available and > none has been selected (neither via `CONF` nor via `SPEC`). Instead of > printing an error message (current behavior) the build system will instead > check if the name of any configuration exactly matches the name of the > checked out Git branch. If so, then that configuration will be built. > > The rationale for this change is that many developers (including me) use > branches to work on multiple things concurrently. Having (at least) one > configuration per branch, named after the branch, is a very natural model > then for setting up one's build configurations. Having the build > configuration corresponding to the checked out Git branch built automatically > is then very convenient (instead of typing `make CONF=$(git branch > --show-current)` every time). > > A couple of design considerations: > - why use `$$(shell command -v git 2>/dev/null)` instead of `$$(GIT)` from > autoconf? Because we don't have a `spec.gmk` available because we haven't > chosen a configuration yet, so we don't have `$$(GIT)`. > - why use `git rev-parse --abbrev-ref HEAD` instead of `git branch > --show-current`? Because `git rev-parse --abbrev-ref` has been around since > [2009](https://github.com/git/git/commit/a45d34691ea624e93863e95706eeb1b1909304f3) > and `git branch --show-current` was introduced in Git 2.22 in 2019 (plus > that `rev-parse --abbrev-ref` works with a detached `HEAD`). `git rev-parse > --is-inside-work-tree` is from > [2007](https://github.com/git/git/commit/892c41b98ae2e6baf3aa13901cb10db9ac67d2f3), > but I think requiring a Git installation more recent than 2009 should be ok. > - why match the branch name exactly instead of a looser matching? This could > be beneficial if branches are named e.g. `$(git branch > --show-current)-fastdebug`, but I wanted to start out with something strict > and then the matching can be made looser if needed. > - is this backwards compatible? Yes, at least I think so. Previously the > build system would return an error but now it might build a configuration, > that seems compatible. The patch does not interfere at all with the code > paths where `CONF` or `SPEC` has been set. > > ### Testing > - [x] Tested locally on macOS/aarch64 and Linux/x64 with and without > configurations matching the named Git branch. > - [x] Tested locally on macOS/aarch64 and Linux/x64 without a Git client. > - [x] Tested locally on macOS/aarch64 a... I'm not sure how useful this is in general as it very much depends on your own style of working. I use branch-based configurations as well but I use a wrapper script to manage that - in part because the wrapper will also run configure if no configuration currently exists. The main limitation I see with the proposal (and I may be misreading it) is that the exact name match seems to preclude working when your configurations have the form `mybranch` and `mybranch-debug` etc. make/InitSupport.gmk line 251: > 249: ifeq ($$(words $$(all_spec_files)), 1) > 250: # We found exactly one configuration, use it > 251: SPECS := $$(strip $$(all_spec_files)) It may be better to keep this as the first action and only do the git checks if necessary. That would minimise potential interference with single branch repos. ------------- PR Review: https://git.openjdk.org/jdk/pull/14202#pullrequestreview-1449929051 PR Review Comment: https://git.openjdk.org/jdk/pull/14202#discussion_r1209607563
