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
