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

Fri Dec 4 09:58:52 UTC 2015

Forgot the link
http://cr.openjdk.java.net/~erikj/8136782/webrev.top.02/

On 2015-12-04 10:57, Erik Joelsson wrote:
> New webrev:
>
> On 2015-12-03 22:01, Magnus Ihse Bursie wrote:
>> 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.
>>
> Good!
>> In basics.m4:
>>
>> The indentation looks weird, but maybe that's just an artifact of 
>> webrev?
> Fixed
>>
>> 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?
>>
> The documentation only says path, which I interpret as either a file 
> or dir.
>> If you needed to check something else than directories, maybe a 
>> different function should be used, like BASIC_FIXUP_EXECUTABLE.
>>
> I needed to (in closed) fix a file that wasn't executable. 
> BASIC_FIXUP_EXECUTABLE doesn't handle that. IMO, the change to support 
> files as well as dirs is better than having to implement a complete 
> new set of macros.
>> 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?
>>
> I reacted to this too when I sorted through the changes I had made in 
> the sandbox, but there was no really good fit. Perhaps basics.m4 is 
> best? Moved 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.
>>
> Fixed
>> > # 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)))
>>
> Fixed
>> 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.
>>
> Right, fixed.
>> 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?
>>
> Fixed the comment
>>  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.
>>
> Sure, didn't know of any other standard, but .config seems to be 
> pretty popular in my home dir.
>> 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?
>>
> Added test for -s.
>> 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.
> In my experience, it's so small the delay is almost not noticeable, 
> even when downloading from west coast US to Stockholm, but I can add 
> some echos. Note that these will not look like other JAB output since 
> the logging framework is not yet available before JAB has even started.
>>
>>    99     gunzip "${installed_jab_script}.gz"
>>
>> Maybe we should verify that gunzip is available?
>>
> Added.
>> 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.
>>
> I did this intentionally. The reason is that stale downloads should 
> not affect usage of the tool. The idea with JAB is to always use the 
> latest version unless specifically told not to. The setup_url is 
> needed to verify that the correct version of JAB is installed. Note 
> that the url can change for a number of reasons, not just the base url 
> part. Imagine someone changing this script to update the jab_revision, 
> you would want that to be reflected next time the script is executed.
>
> Note that in the normal case, where you supply JAB_SERVER, that part 
> of the URL is not saved in the data file, so leaving it out next time 
> will not trigger a new download.
>> 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?
>>
> Done
>> Since the script is storing stuff in .jab, maybe you should add .jab 
>> to .hgignore?
>>
> Forgot to bring that change over, added.
>> 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?
>>
> Fixed
>> // 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".
>>
> A discussion with Stefan, who is building the tool consuming this 
> particular attribute, he preferred labels.
>>  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?
>>
> Fixed
>>  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.
>>
> Clarified
>>  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. :)
>>
> Clarified
>
> /Erik
>> /Magnus
>>
>