[rfc][icedtea-web] Generate Bash Completion file
Jiri Vanek
jvanek at redhat.com
Fri Jun 19 11:03:28 UTC 2015
On 06/18/2015 06:11 PM, Lukasz Dracz wrote:> Hello,
>
> I have included an updated patch, that is not finished and doesn't work at the moment.
> I have a few questions.
>
> I think the main question is what generates all the install-am and install-*Something*-am 's that then reference something
> like install-*SOMETHING*-local from the makefile.am. I can't seem to figure this part. I can see that the automake that generates the .in file from the .am
> adds these in but what tells automake to do this ? Is there a convention to follow that once followed means automake will do this ?
Isn't the way of generated files obvious in our Makefile.am?
>
> ----- Original Message -----
>> From: "Jiri Vanek" <jvanek at redhat.com>
>> To: "IcedTea Distro List" <distro-pkg-dev at openjdk.java.net>, "Lukasz Dracz" <ldracz at redhat.com>,
>> aazores at icedtea.classpath.org
>> Sent: Sunday, June 14, 2015 2:06:10 PM
>> Subject: re: [rfc][icedtea-web] Generate Bash Completion file
>>
>>
>>
>>
>> hi!
>>
>> A lot of nits just from quick web based reading.
>>
>>
>> diff --git a/Makefile.am b/Makefile.am
>> --- a/Makefile.am
>> +++ b/Makefile.am
>> @@ -259,13 +259,13 @@
>> check-local: $(RHINO_TESTS) $(JUNIT_TESTS)
>>
>> clean-local: clean-netx clean-plugin clean-liveconnect \
>> - clean-native-ecj clean-launchers clean-desktop-files clean-docs
>> clean-generated-docs clean-tests clean-bootstrap-directory
>> + clean-native-ecj clean-launchers clean-desktop-files clean-docs
>> clean-generated-docs clean-generated-bash-completion clean-tests
>> clean-bootstrap-directory
>> if [ -e stamps ] ; then \
>> rmdir stamps ; \
>> fi
>>
>> .PHONY: clean-IcedTeaPlugin clean-add-netx clean-add-netx-debug
>> clean-add-plugin clean-add-plugin-debug \
>> - clean-bootstrap-directory clean-native-ecj clean-desktop-files
>> clean-netx-docs clean-docs clean-plugin-docs clean-generated-docs \
>> + clean-bootstrap-directory clean-native-ecj clean-desktop-files
>> clean-netx-docs clean-docs clean-plugin-docs clean-generated-docs
>> clean-generated-bash-completion \
>> clean-tests check-local clean-launchers stamps/check-pac-functions.stamp
>> stamps/run-netx-unit-tests.stamp clean-netx-tests \
>> clean-junit-runner clean-netx-unit-tests
>>
>> @@ -504,6 +504,12 @@
>> sed -i '/RhinoBasedPacEvaluator/ d' $@
>> endif
>>
>> +stamps/generate-bash-completion.stamp: stamps/netx.stamp
Agaiun - why stamp??? teh generated file IS the "stamp" and THE target
>> + BC_COMMAND="$(SYSTEM_JRE_DIR)/bin/java -cp $(NETX_DIR)
>> net.sourceforge.jnlp.OptionsDefinitions" ; \
>> + $$BC_COMMAND; \
>> + sudo mv icedteaweb-completion /etc/bash_completion.d/ ; \
>>
>> This is absolyutely no go. You must split this to two targets
>> - first during make. during make you generate this file o BUILD_DIR
>> - second, during install, you install the genrated file to
>> PREFIX/etc/bash_completion.d/icedteaweb-completion
>
>
>
>> You do not need to handle permissions - user decide in configure time what is
>> PREFIX, and then in install time he use root or not, acording to his prefix.
>> By default prefix is "/" so by default you hav eto be root to call make
>> install.
>> But only make install. not make.
>
> What is the point of PREFIX ? is it for if the user has /etc/bash_comletion.d/ located under a folder
> something like /randomDir/etc/bash_completion.d/ or does it have something to do with permissions ?
oh dear ;(((
yes, prefix is exacrtly fo this.
It msut be clear from "/" being default prefix, or not?
If you during configure set your own porefix, you expect application ends in in prefix. Nowehre else.
If you then see IN_YOUR_PREFIX/etc/somefile you can gues that somefile belongs to /etc/ - check it
and palce it here or link on your own.
How are you testingitw??? You are not using custom prefix???? So you are actually runnint the make
install as root? Oh dear :(
>
>> + touch $@ ;
>> +
>> stamps/generate-docs.stamp: stamps/netx.stamp
>> mkdir -p "$(DOCS_DIR)" ; \
>> HTML_DOCS_TARGET_DIR="$(DOCS_DIR)/html" ; \
>> @@ -571,7 +577,7 @@
>> mkdir -p stamps
>> touch $@
>>
>>
>> acordingly the clean phases have to be splited.
>>
>> By the way - make install is generated target. Yo just add generated
>> icedteaweb-completion to list of cared files.
>
> Where do I add to this list ?
>
>> Also, please dont use stamps fo simple generation. The target is file itself.
>> ( see lower)
>
> Okay
>
>> -stamps/netx-dist.stamp: stamps/netx.stamp $(abs_top_builddir)/netx.manifest
>> stamps/generate-docs.stamp
>> +stamps/netx-dist.stamp: stamps/netx.stamp $(abs_top_builddir)/netx.manifest
>> stamps/generate-docs.stamp stamps/generate-bash-completion.stamp stamps/
>> (cd $(NETX_DIR) ; \
>> mkdir -p lib ; \
>> $(SYSTEM_JDK_DIR)/bin/jar cfm lib/classes.jar \
>> @@ -686,6 +692,8 @@
>> rm -rf "$(DOCS_DIR)"
>> rm -f stamps/generate-docs.stamp
>>
>> +clean-generated-bash-completion:
>> + rm -rf stamps/generate-bash-completion.stamp
>>
>> Ok. Here is the bug. What the target is cleaning? It is removing stamp! not
>> generated file!
>
> Okay use of stamps removed.
>
>> # check
>> # ==========================
>> diff --git a/icedteaweb-completion b/icedteaweb-completion
>> deleted file mode 100644
>> --- a/icedteaweb-completion
>> +++ /dev/null
>>
>> Hmm... Why to remove. The file in javacode is absolutely unreadable. I would
>> rater move this file to
>> icedteaweb-completion.in and durng generation just replaced the stings ifn
>> "opts"
>
> Okay moved to icedteaweb-completion.in and added one "-verbose" flag to policyeditor that I noticed was added.
So other then -verbose, the file is same? hmm
>
>> >@@ -1,44 +0,0 @@
>> >-#/bin/bash
>> >-#place this file over to/etc/bash_completion.d/ to make this file useful
>> >-#Note: If you do not have bash-completion you will need to install it
>> >-_itweb-settings()
>> >-{
>> >- local cur prev opts base
>> >- cur="${COMP_WORDS[COMP_CWORD]}"
>> >- prev="${COMP_WORDS[COMP_CWORD-1]}"
>> >-
>> >- # Icedtea-web settings Options
>> >- opts="-check -get -headless -help -info -list -reset -set -verbose"
>> >-
>> >- COMPREPLY=($(compgen -W "${opts}" -- ${cur}))
>> >- return 0
>> >-}
>> >-complete -F _itweb-settings itweb-settings
>> >-
>> >-_policyeditor()
>> >-{
>> >- local cur prev opts base
>> >- cur="${COMP_WORDS[COMP_CWORD]}"
>> >- prev="${COMP_WORDS[COMP_CWORD-1]}"
>> >-
>> >- # PolicyEditor Options
>> >- opts="-codebase -file -help"
>> >-
>> >- COMPREPLY=($(compgen -W "${opts}" -- ${cur}))
>> >- return 0
>> >-}
>> >-complete -F _policyeditor policyeditor
>> >-
>> >-_javaws()
>> >-{
>> >- local cur prev opts base
>> >- cur="${COMP_WORDS[COMP_CWORD]}"
>> >- prev="${COMP_WORDS[COMP_CWORD-1]}"
>> >-
>> >- # JavaWs Options
>> >- opts="-about -help -license -viewer -Xclearcache -allowredirect -arg
>> >-headless -html -jnlp -nosecurity -noupdate -param -property -strict -update
>> >-verbose -version -Xignoreheaders -xml -Xnofork -Xoffline -Xtrustnone"
>> >-
>> >- COMPREPLY=($(compgen -W "${opts}" -- ${cur}))
>> >- return 0
>> >-}
>> >-complete -F _javaws javaws
>> >diff --git a/netx/net/sourceforge/jnlp/OptionsDefinitions.java
>> >b/netx/net/sourceforge/jnlp/OptionsDefinitions.java
>> >--- a/netx/net/sourceforge/jnlp/OptionsDefinitions.java
>> >+++ b/netx/net/sourceforge/jnlp/OptionsDefinitions.java
>> >@@ -36,6 +36,9 @@
>> > */
>> > package net.sourceforge.jnlp;
>> >
>> >+import java.io.File;
>> >+import java.io.IOException;
>> >+import java.nio.file.Files;
>> > import java.util.ArrayList;
>> > import java.util.Arrays;
>> > import java.util.List;
>> >@@ -223,5 +226,65 @@
>> > return l;
>> > }
>> >
>> >-
>> >+ public static void main(String [ ] args) throws IOException {
>> >+ makeTabCompletionFile();
>> >+ }
>> >+
>> >+ public static String transformToLine (List<OPTIONS> options) {
>> >+ String line = "";
>> >+ for (OPTIONS option : options) {
>> >+ line = line + option.option + " ";
>> >+ }
>> >+ return line;
>> >+ }
>> >+ public static void makeTabCompletionFile() throws IOException {
>> >
>> >Is there any difference against orignal file? if no,please use .in format.
>> >
>> >If yes, then please split this patch to two parts. Firt, replace already
>> >pushed file by this new powerful (and it will be valid also for 1.6!) and as
>> >second patch, chnage it to .in form, and proceed with generation.
> No difference other than the -verbose flag I noticed was missing.
>
> Thanks for the review !
>
> Regards,
> Lukasz Dracz
>
>
> generate-bash-completion-4.patch
>
>
> diff --git a/Makefile.am b/Makefile.am
> --- a/Makefile.am
> +++ b/Makefile.am
> @@ -302,6 +302,9 @@
> endif
> endif
>
> +install-bash-local:
> + mv icedteaweb-completion /etc/bash_completion.d/
> +
uh....
mv icedteaweb-completion $(DESTDIR)/etc/bash_completion.d/
and don't forget to ensure existence of that dir.
also - what is usage of this target? It never will be called. This line belongs to install target.
> # all generated manpages must be removed one by one
> uninstall-local:
> rm -f $(DESTDIR)$(libdir)/$(BUILT_PLUGIN_LIBRARY)
> @@ -504,6 +507,13 @@
> sed -i '/RhinoBasedPacEvaluator/ d' $@
> endif
Here is missng uninstall of your file...
>
> +generate-bash-completion:
> + BC_COMMAND="$(SYSTEM_JRE_DIR)/bin/java -cp $(NETX_DIR) net.sourceforge.jnlp.OptionsDefinitions" ; \
We have standartized macro to construct classapth.
> + $$BC_COMMAND;
For the usage see the bottom. I really have strong agasnt against this approach:(
> +
> +install-bash-completion:
> + mv icedteaweb-completion/etc/bash_completion.d/
Never duplicate code. Call. And anyway, both those targets are wrong.
> +
It sa second time here??
the
uh....
mv icedteaweb-completion $(DESTDIR)/etc/bash_completion.d/
and don't forget to ensure existence of that dir.
will eb avlid anyway
> stamps/generate-docs.stamp: stamps/netx.stamp
> mkdir -p "$(DOCS_DIR)" ; \
> HTML_DOCS_TARGET_DIR="$(DOCS_DIR)/html" ; \
> @@ -571,7 +581,7 @@
> mkdir -p stamps
> touch $@
>
> -stamps/netx-dist.stamp: stamps/netx.stamp $(abs_top_builddir)/netx.manifest stamps/generate-docs.stamp
> +stamps/netx-dist.stamp: stamps/netx.stamp $(abs_top_builddir)/netx.manifest stamps/generate-docs.stamp generate-bash-completion
The generation is 100% not part of netx.jar. Pelase find better target.
The docs are here, because half of them is included direltly in netx.jar
> (cd $(NETX_DIR) ; \
> mkdir -p lib ; \
> $(SYSTEM_JDK_DIR)/bin/jar cfm lib/classes.jar \
> @@ -686,7 +696,6 @@
> rm -rf "$(DOCS_DIR)"
> rm -f stamps/generate-docs.stamp
>
> -
> # check
> # ==========================
>
> diff --git a/icedteaweb-completion b/icedteaweb-completion.in
> rename from icedteaweb-completion
> rename to icedteaweb-completion.in
> --- a/icedteaweb-completion
> +++ b/icedteaweb-completion.in
> @@ -22,7 +22,7 @@
> prev="${COMP_WORDS[COMP_CWORD-1]}"
>
> # PolicyEditor Options
> - opts="-codebase -file -help"
> + opts="-codebase -file -help -verbose"
>
> COMPREPLY=($(compgen -W "${opts}" -- ${cur}))
> return 0
> @@ -37,7 +37,6 @@
>
> # JavaWs Options
> opts="-about -help -license -viewer -Xclearcache -allowredirect -arg -headless -html -jnlp -nosecurity -noupdate -param -property -strict -update -verbose -version -Xignoreheaders -xml -Xnofork -Xoffline -Xtrustnone"
> -
> COMPREPLY=($(compgen -W "${opts}" -- ${cur}))
> return 0
> }
> diff --git a/netx/net/sourceforge/jnlp/OptionsDefinitions.java b/netx/net/sourceforge/jnlp/OptionsDefinitions.java
> --- a/netx/net/sourceforge/jnlp/OptionsDefinitions.java
> +++ b/netx/net/sourceforge/jnlp/OptionsDefinitions.java
> @@ -1,4 +1,4 @@
> -/*
> +/*
> Copyright (C) 2008 Red Hat, Inc.
>
> This file is part of IcedTea.
> @@ -36,12 +36,20 @@
> */
> package net.sourceforge.jnlp;
>
> +import java.io.BufferedReader;
> +import java.io.File;
> +import java.io.FileReader;
> +import java.io.IOException;
> +import java.nio.file.Files;
> import java.util.ArrayList;
> import java.util.Arrays;
> import java.util.List;
>
> import static net.sourceforge.jnlp.runtime.Translator.R;
>
> +import net.sourceforge.jnlp.config.InfrastructureFileDescriptor;
> +import net.sourceforge.jnlp.config.PathsAndFiles;
> +
> public class OptionsDefinitions {
>
> public static enum OPTIONS {
> @@ -223,5 +231,46 @@
> return l;
> }
>
> -
> + public static void main(String [ ] args) throws IOException {
> + replaceOptionsInTabCompletionFile();
> + }
> +
> + public static String transformToLine (List<OPTIONS> options) {
> + String line = "";
> + for (OPTIONS option : options) {
> + line = line + option.option + " ";
> + }
nit - use stringbuilder rather. Also please keep the String without trailing space.
Also please, add unit test fo this method. It is crucialmethod of the approach. Also made it
protected or package private.
> + return line;
> + }
> +
> + public static void replaceOptionsInTabCompletionFile() throws IOException {
Also this methdo protected or package rpivate + test.. But I would liek to encourage you to go
different way ( se bellow)
Also - if you proeprly test this method, it will force you to decompose it, and so you will figure
out why I'm forcing you to do some things.
> + String opts = " opts=\"";
> + File inputCompletionFile = new File("../icedteaweb-completion.in");
If you will continue with this approach, dont hardcode file, pass it as argument. Still I wiould
like to discourage you. See bottom.
> + File completionFile = new File("icedteaweb-completion");
> + FileReader fileReader = new FileReader(inputCompletionFile);
> + BufferedReader bufferedReader = new BufferedReader(fileReader);
> +
> + String body = "";
> + String line = bufferedReader.readLine();
> + String options = "";
> + while(line != null) {
> + if (line.contains("itweb-settings")) {
> + options = transformToLine(getItwsettingsCommands());
> + } else if (line.contains("policyeditor")) {
> + options = transformToLine(getPolicyEditorOptions());
> + } else if (line.contains("javaws")) {
> + options = transformToLine(getJavaWsOptions());
> + }
> +
> + if (line.contains(opts)) {
> + body = body + opts + options + "\"\n";
> + } else {
> + body = body + line + "\n";
> + }
> + line = bufferedReader.readLine();
> + }
> + bufferedReader.close();
> +
> + Files.write(completionFile.toPath(), body.getBytes());
> + }
> }
>
Althoug this approach is much more better then the one before, its quite unreadable.
On one side I appreciate fresh mind, on second... I really think the standard steps are much more
versatile and stable and understandable.
s:
1) the main method of OptionsDefinitions will take exactly one parameter javaws itweb-settings or
policyeditor
2) according to it, it will print to *stdout* options for selcted program
3) the in file will be readable - so it will have correct opts="@PROGRAM@" syntaxe
4) code in makefile will get this output, and substitue @PROGRAM@ (eg @JAVAWS@ in .in file by given
value.
You will see that your code will be much shorter (main method about lines and one line in makefile
for generation, one for install definiton, and 5 lines of options to string method.
Tests may be longer. *but* tests to "my" approach are much shorter then for "yours" approach.
Sorry for so much feedback :(
J.
More information about the distro-pkg-dev
mailing list