[rfc][icedtea-web] Generate Bash Completion file
Jiri Vanek
jvanek at redhat.com
Wed Aug 26 08:14:04 UTC 2015
Hi Lukasz!
Tahnk you for your email. Here is, according to yours input, finished version which I'm going to push.
tahnk you again!
J.
On 06/19/2015 01:03 PM, Jiri Vanek wrote:
> 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.
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: generation-Of-Bash-Completion4.patch
Type: text/x-patch
Size: 8452 bytes
Desc: not available
URL: <http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20150826/52202b2c/generation-Of-Bash-Completion4.patch>
More information about the distro-pkg-dev
mailing list