RFR: JDK-8136782: Introduce a build/configure wrapper

Thu Dec 3 21:01:26 UTC 2015

On 2015-12-03 16:27, Erik Joelsson wrote:
> Hello,
>
> This patch adds support for a new Oracle internal tool that will help 
> us manage build configurations. I doubt it could be of any use to 
> outside users, except perhaps to give a bit of insight into what 
> versions of certain dependencies we use internally, as those will be 
> specified in the jab-profiles.js configuration file.
>
> In addition to the JAB configuration files added here, I've also added 
> a new configure option --with-default-make-target with which you can 
> change the default make target for a configuration. A bug is fixed in 
> the configure wrapper script where it's currently not propagating the 
> return value of configure. Finally a new make command line variable is 
> added, CONF_NAME, which works like CONF, except it only matches exact 
> configuration names.
>
> Bug: https://bugs.openjdk.java.net/browse/JDK-8136782
> Webrev: http://cr.openjdk.java.net/~erikj/8136782/webrev.top.01/
Hi Erik,

Nice to see progress on this new feature. :)

I have a bunch of comments this time.

In basics.m4:

The indentation looks weird, but maybe that's just an artifact of webrev?

I'm a bit curious why this fix was needed. I thought one of the idea 
with BASIC_FIXUP_PATH was to guarantee that we pointed to an existing 
directory?

If you needed to check something else than directories, maybe a 
different function should be used, like BASIC_FIXUP_EXECUTABLE.

Does --with-default-make-target really belong in build-performance? I'm 
not sure really where it belongs, though. It's more closely related to 
the stuff in source-dirs.m4, but the name of that file makes it too 
narrow. Maybe rename it to sources.m4 and put it there?

In InitSupport.gmk:
Maybe you should check so not both CONF and CONF_NAME is used as well?

Also make/Help.gmk should be updated with CONF_NAME, I think.

 > # Create a SPEC definition. This will contain the path to one or more 
spec.gmk files.
 > SPECS := $$(addsuffix /spec.gmk, $$(addprefix $$(build_dir)/, 
$$(matching_confs)))

Actually, no. It will contain only one. In fact, maybe you should add a 
sanity  check that  matching_confs contains exactly one configuration 
(unless it's empty).  And, given that, the SPECS assignment can be 
simplified to just simple string concatenation.

In jab.sh:

  25 # This script installs the jdk build wrapper tool into it's own local
   26 # repository and puts wrapper scripts into this bin directory.

"installs the jab tool" perhaps sounds better?

  35     if [ -f "~/.jab" ]; then
   36         source ~/.jab

Is this a local configuration file? Why not .config/jab/jab.conf 
instead? I'd like to keep my home dir clutter free, if possible.

   97     ${getcmd} ${jab_url} > "${installed_jab_script}.gz"
   98     rm -f "${installed_jab_script}"
   99     gunzip "${installed_jab_script}.gz"

Maybe rm -f the  "${installed_jab_script}.gz" first as well, just to be 
on the safe side? Also, since network connectivity is the most likely 
cause of failure here, maybe explicitely test that 
"${installed_jab_script}.gz" arrived and is non-zero, and otherwise 
print a clear error message stating the URL that we tried to access and 
say that it failed. And/or check the exit code from wget/curl?

Also, when installing jab, maybe you could be slightly more verbose? 
Like, "Downloading jab.sh.gz" and "Extracting jab.sh.gz", just to let 
the user know something is happening. And while jab.sh.gz might be 
likely to be small, on a slow line it might still be a noticable delay.

    99     gunzip "${installed_jab_script}.gz"

Maybe we should verify that gunzip is available?

Do we need to run setup_url() even if not installing?

I'm also not sure about the "Install url changed" thingy. If I've once 
downloaded jab using JAB_SERVER=foo jab.sh, then I should be able to 
leave JAB_SERVER=foo out on future invocations, methinks. But this logic 
looks like it's going to trip that up. I'm not sure for what reason.

Finally, shell scripts with functions can be bit a hard to read. Maybe 
you could add some kind of delimiter after the install_jab() function 
and before the main body of the file?

Since the script is storing stuff in .jab, maybe you should add .jab to 
.hgignore?

In jab-profiles.js:

   45  * The parameter 'input' is an object that optionally contains the
   46  * some data.

"the some"  --> "some".

*   output_basedir; "build",

Perhaps colon?

// List of free from labels describing aspects of this profile
  115  *       labels: <Array of strings>

"free form". But why not "tags" instead of "labels"? I'd say that in 
modern usage, "tag" means just "free form label".

  132  *       // List of configure args to add when using this dependency,
  133  *       // defaults to input.get("<dependency-name", "install_path")

Seems a "--with-<dependency-name>=" got lost there, somehow?

  135  *       // Name of environment variable to set when using this 
dependency
  136  *       environment_name: <string>
  137  *       // Value of environment variable to set when using this 
dependency
  138  *       environment_value: <string>
  139  *       // Value to add to the PATH variable when using this 
dependency
  140  *       environment_path: <string>

This is set for "jab make" only, right? Maybe clarify that.

  144  *       // For certain dependencies where a legacy distribution 
mechanism is
  145  *       // already i place, the "javare" server layout is also 
supported

  "in place". Also, missing period.

  150  *       // buildnumber (optional), files and checksumfile is 
possible for

  "build number", "checksum file"

   *       // aritfacts following the standard layout.

"artifacts"

   *       // For other files, use checksumpath and paths instead

"checksum path"

   62  * input.build_unix_os
   63  * input.build_unix_cpu
   66  * input.build_unix_platform

What is build_unix_cpu, etc? Sounds fully incomprehensible to me. :)

/Magnus