| Message ID | 20220513010740.8544-3-cross@oxidecomputer.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | kvm-unit-tests: Build changes to support illumos. | expand |
Adding the official KUT maintainers, they undoubtedly know more about the getopt stuff than me. On Fri, May 13, 2022, Dan Cross wrote: > This change modifies the `configure` script to run under illumos Nit, use imperative mood. KUT follows the kernel's rules/guidelines for the most part. From Linux's Documentation/process/submitting-patches.rst: Describe your changes in imperative mood, e.g. "make xyzzy do frotz" instead of "[This patch] makes xyzzy do frotz" or "[I] changed xyzzy to do frotz", as if you are giving orders to the codebase to change its behaviour. E.g. Exempt illumos, which reports itself as SunOS, from the `getopt -T` check for enhanced getopt. blah blah blah... > by not probing for, `getopt -T` (illumos `getopt` supports the > required functionality, but exits with a different return status > when invoked with `-T`). > > Signed-off-by: Dan Cross <cross@oxidecomputer.com> > --- > configure | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/configure b/configure > index 86c3095..7193811 100755 > --- a/configure > +++ b/configure > @@ -15,6 +15,7 @@ objdump=objdump > ar=ar > addr2line=addr2line > arch=$(uname -m | sed -e 's/i.86/i386/;s/arm64/aarch64/;s/arm.*/arm/;s/ppc64.*/ppc64/') > +os=$(uname -s) > host=$arch > cross_prefix= > endian="" > @@ -317,9 +318,9 @@ EOF > rm -f lib-test.{o,S} > fi > > -# require enhanced getopt > +# require enhanced getopt everywhere except illumos > getopt -T > /dev/null > -if [ $? -ne 4 ]; then > +if [ $? -ne 4 ] && [ "$os" != "SunOS" ]; then What does illumos return for `getopt -T`? Unless it's a direct collision with the "old" getopt, why not check for illumos' return? The SunOS check could be kept (or not). E.g. IMO this is much more self-documenting (though does $? get clobbered by the check? I'm terrible at shell scripts...). if [ $? -ne 4 ] && [ "$os" != "SunOS" || $? -ne 666 ]; then Test if your getopt(1) is this enhanced version or an old version. This generates no output, and sets the error status to 4. Other implementations of getopt(1), and this version if the environment variable GETOPT_COMPATIBLE is set, will return '--' and error status 0. > echo "Enhanced getopt is not available, add it to your PATH?" > exit 1 > fi > -- > 2.31.1 >
On 13/05/2022 16.34, Sean Christopherson wrote: > Adding the official KUT maintainers, they undoubtedly know more about the getopt > stuff than me. > > On Fri, May 13, 2022, Dan Cross wrote: >> This change modifies the `configure` script to run under illumos > > Nit, use imperative mood. KUT follows the kernel's rules/guidelines for the most > part. From Linux's Documentation/process/submitting-patches.rst: > > Describe your changes in imperative mood, e.g. "make xyzzy do frotz" > instead of "[This patch] makes xyzzy do frotz" or "[I] changed xyzzy > to do frotz", as if you are giving orders to the codebase to change > its behaviour. > > > E.g. > > Exempt illumos, which reports itself as SunOS, from the `getopt -T` check > for enhanced getopt. blah blah blah... > >> by not probing for, `getopt -T` (illumos `getopt` supports the >> required functionality, but exits with a different return status >> when invoked with `-T`). >> >> Signed-off-by: Dan Cross <cross@oxidecomputer.com> >> --- >> configure | 5 +++-- >> 1 file changed, 3 insertions(+), 2 deletions(-) >> >> diff --git a/configure b/configure >> index 86c3095..7193811 100755 >> --- a/configure >> +++ b/configure >> @@ -15,6 +15,7 @@ objdump=objdump >> ar=ar >> addr2line=addr2line >> arch=$(uname -m | sed -e 's/i.86/i386/;s/arm64/aarch64/;s/arm.*/arm/;s/ppc64.*/ppc64/') >> +os=$(uname -s) >> host=$arch >> cross_prefix= >> endian="" >> @@ -317,9 +318,9 @@ EOF >> rm -f lib-test.{o,S} >> fi >> >> -# require enhanced getopt >> +# require enhanced getopt everywhere except illumos >> getopt -T > /dev/null >> -if [ $? -ne 4 ]; then >> +if [ $? -ne 4 ] && [ "$os" != "SunOS" ]; then > > What does illumos return for `getopt -T`? Unless it's a direct collision with > the "old" getopt, why not check for illumos' return? The SunOS check could be > kept (or not). E.g. IMO this is much more self-documenting (though does $? get > clobbered by the check? I'm terrible at shell scripts...). According to https://illumos.org/man/1/getopt : NOTES getopt will not be supported in the next major release. ... So even if we apply this fix now, this will likely break soon again. Is there another solution to this problem? Thomas
On Thu, May 19, 2022 at 5:53 AM Thomas Huth <thuth@redhat.com> wrote: > On 13/05/2022 16.34, Sean Christopherson wrote: > > [snip] > > According to https://illumos.org/man/1/getopt : > > NOTES > > getopt will not be supported in the next major release. > ... > > So even if we apply this fix now, this will likely break soon again. Is > there another solution to this problem? I wouldn't put too much stock into that; that note came from Solaris and has been in the man page for 30 years or more [*]. I think there are too many shell scripts in the wild using `getopt` for it to ever be removed. Indeed, if anything this highlights something to clean up on the illumos side by removing that from the man page. - Dan C. [*] e.g. https://www.freebsd.org/cgi/man.cgi?query=getopt&apropos=0&sektion=0&manpath=SunOS+5.5.1&arch=default&format=html from 1992!
On Fri, May 13, 2022 at 10:35 AM Sean Christopherson <seanjc@google.com> wrote: > Adding the official KUT maintainers, they undoubtedly know more about the getopt > stuff than me. Thanks. > On Fri, May 13, 2022, Dan Cross wrote: > > This change modifies the `configure` script to run under illumos > > Nit, use imperative mood. KUT follows the kernel's rules/guidelines for the most > part. From Linux's Documentation/process/submitting-patches.rst: > [snip] Done locally, but see below. > > diff --git a/configure b/configure > > index 86c3095..7193811 100755 > > --- a/configure > > +++ b/configure > > @@ -15,6 +15,7 @@ objdump=objdump > > ar=ar > > addr2line=addr2line > > arch=$(uname -m | sed -e 's/i.86/i386/;s/arm64/aarch64/;s/arm.*/arm/;s/ppc64.*/ppc64/') > > +os=$(uname -s) > > host=$arch > > cross_prefix= > > endian="" > > @@ -317,9 +318,9 @@ EOF > > rm -f lib-test.{o,S} > > fi > > > > -# require enhanced getopt > > +# require enhanced getopt everywhere except illumos > > getopt -T > /dev/null > > -if [ $? -ne 4 ]; then > > +if [ $? -ne 4 ] && [ "$os" != "SunOS" ]; then > > What does illumos return for `getopt -T`? Sadly, it returns "0". I was wrong in my earlier explorations because I did not realize that `configure` does not use `getopt` aside from that one check, which is repeated in `run_tests.sh`. I would argue that the most straight-forward way to deal with this is to just remove the check for "getopt" from "configure", which doesn't otherwise use "getopt". The only place it is used is in `run_tests.sh`, which is unlikely to be used directly for illumos, and repeats the check anyway. > Unless it's a direct collision with > the "old" getopt, why not check for illumos' return? It collides with "legacy" getopt. :-( > The SunOS check could be > kept (or not). E.g. IMO this is much more self-documenting (though does $? get > clobbered by the check? I'm terrible at shell scripts...). I think it is more or less moot now, but "$?" does get clobbered by the check. > if [ $? -ne 4 ] && [ "$os" != "SunOS" || $? -ne 666 ]; then > > Test if your getopt(1) is this enhanced version or an old version. This > generates no output, and sets the error status to 4. Other implementations of > getopt(1), and this version if the environment variable GETOPT_COMPATIBLE is > set, will return '--' and error status 0. Sigh. That's precisely what the illumos version does. :-( But it's perhaps worth pointing out that `getopt` isn't used by `configure` aside from just that check. Does it, perhaps, make more sense just to remove it from `configure` entirely? - Dan C.
On 24/05/2022 23.20, Dan Cross wrote: > On Thu, May 19, 2022 at 5:53 AM Thomas Huth <thuth@redhat.com> wrote: >> On 13/05/2022 16.34, Sean Christopherson wrote: >>> [snip] >> >> According to https://illumos.org/man/1/getopt : >> >> NOTES >> >> getopt will not be supported in the next major release. >> ... >> >> So even if we apply this fix now, this will likely break soon again. Is >> there another solution to this problem? > > I wouldn't put too much stock into that; that note came from Solaris > and has been in the man page for 30 years or more [*]. :-) > I think there > are too many shell scripts in the wild using `getopt` for it to ever be > removed. Indeed, if anything this highlights something to clean up > on the illumos side by removing that from the man page. Yup, cleaning up that man page sounds like a good idea then, indeed. Thomas
On 24/05/2022 23.22, Dan Cross wrote: > On Fri, May 13, 2022 at 10:35 AM Sean Christopherson <seanjc@google.com> wrote: ... >>> diff --git a/configure b/configure >>> index 86c3095..7193811 100755 >>> --- a/configure >>> +++ b/configure >>> @@ -15,6 +15,7 @@ objdump=objdump >>> ar=ar >>> addr2line=addr2line >>> arch=$(uname -m | sed -e 's/i.86/i386/;s/arm64/aarch64/;s/arm.*/arm/;s/ppc64.*/ppc64/') >>> +os=$(uname -s) >>> host=$arch >>> cross_prefix= >>> endian="" >>> @@ -317,9 +318,9 @@ EOF >>> rm -f lib-test.{o,S} >>> fi >>> >>> -# require enhanced getopt >>> +# require enhanced getopt everywhere except illumos >>> getopt -T > /dev/null >>> -if [ $? -ne 4 ]; then >>> +if [ $? -ne 4 ] && [ "$os" != "SunOS" ]; then >> >> What does illumos return for `getopt -T`? > > Sadly, it returns "0". I was wrong in my earlier explorations > because I did not realize that `configure` does not use `getopt` > aside from that one check, which is repeated in `run_tests.sh`. > > I would argue that the most straight-forward way to deal with > this is to just remove the check for "getopt" from "configure", > which doesn't otherwise use "getopt". The only place it is > used is in `run_tests.sh`, which is unlikely to be used directly > for illumos, and repeats the check anyway. Fine for me if we remove the check from configure, or turn it into a warning instead ("Enhanced getopt is not available, you won't be able to use the run_tests.sh script" or so). Thomas
On Wed, May 25, 2022 at 09:44:33AM +0200, Thomas Huth wrote: > On 24/05/2022 23.22, Dan Cross wrote: > > On Fri, May 13, 2022 at 10:35 AM Sean Christopherson <seanjc@google.com> wrote: > ... > > > > diff --git a/configure b/configure > > > > index 86c3095..7193811 100755 > > > > --- a/configure > > > > +++ b/configure > > > > @@ -15,6 +15,7 @@ objdump=objdump > > > > ar=ar > > > > addr2line=addr2line > > > > arch=$(uname -m | sed -e 's/i.86/i386/;s/arm64/aarch64/;s/arm.*/arm/;s/ppc64.*/ppc64/') > > > > +os=$(uname -s) > > > > host=$arch > > > > cross_prefix= > > > > endian="" > > > > @@ -317,9 +318,9 @@ EOF > > > > rm -f lib-test.{o,S} > > > > fi > > > > > > > > -# require enhanced getopt > > > > +# require enhanced getopt everywhere except illumos > > > > getopt -T > /dev/null > > > > -if [ $? -ne 4 ]; then > > > > +if [ $? -ne 4 ] && [ "$os" != "SunOS" ]; then > > > > > > What does illumos return for `getopt -T`? > > > > Sadly, it returns "0". I was wrong in my earlier explorations > > because I did not realize that `configure` does not use `getopt` > > aside from that one check, which is repeated in `run_tests.sh`. > > > > I would argue that the most straight-forward way to deal with > > this is to just remove the check for "getopt" from "configure", > > which doesn't otherwise use "getopt". The only place it is > > used is in `run_tests.sh`, which is unlikely to be used directly > > for illumos, and repeats the check anyway. > > Fine for me if we remove the check from configure, or turn it into a warning > instead ("Enhanced getopt is not available, you won't be able to use the > run_tests.sh script" or so). > Ack for simply changing the configure error to a warning for now. Ideally we'd limit the dependencies this project has though. So maybe rewriting the run_tests.sh command line parser without getopt would be the better thing to do. Thanks, drew
We have begun using kvm-unit-tests to test Bhyve under illumos. We started by cross-compiling the tests on Linux and transfering the binary artifacts to illumos machines, but it proved more convenient to build them directly on illumos. This patch series modifies the build infrastructure to allow building on illumos; the changes were minimal. I have also tested these changes on Linux to ensure no regressions. Dan Cross (2): kvm-unit-tests: invoke $LD explicitly in kvm-unit-tests: configure changes for illumos. configure | 5 +++-- x86/Makefile.common | 6 +++--- 2 files changed, 6 insertions(+), 5 deletions(-)
On Thu, May 26, 2022 at 3:12 AM Andrew Jones <drjones@redhat.com> wrote: > On Wed, May 25, 2022 at 09:44:33AM +0200, Thomas Huth wrote: > > [snip] > > Fine for me if we remove the check from configure, or turn it into a warning > > instead ("Enhanced getopt is not available, you won't be able to use the > > run_tests.sh script" or so). > > Ack for simply changing the configure error to a warning for now. Ideally > we'd limit the dependencies this project has though. So maybe rewriting > the run_tests.sh command line parser without getopt would be the better > thing to do. Sounds good. New patches inbound. Thank you! - Dan C.
diff --git a/configure b/configure index 86c3095..7193811 100755 --- a/configure +++ b/configure @@ -15,6 +15,7 @@ objdump=objdump ar=ar addr2line=addr2line arch=$(uname -m | sed -e 's/i.86/i386/;s/arm64/aarch64/;s/arm.*/arm/;s/ppc64.*/ppc64/') +os=$(uname -s) host=$arch cross_prefix= endian="" @@ -317,9 +318,9 @@ EOF rm -f lib-test.{o,S} fi -# require enhanced getopt +# require enhanced getopt everywhere except illumos getopt -T > /dev/null -if [ $? -ne 4 ]; then +if [ $? -ne 4 ] && [ "$os" != "SunOS" ]; then echo "Enhanced getopt is not available, add it to your PATH?" exit 1 fi
This change modifies the `configure` script to run under illumos by not probing for, `getopt -T` (illumos `getopt` supports the required functionality, but exits with a different return status when invoked with `-T`). Signed-off-by: Dan Cross <cross@oxidecomputer.com> --- configure | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)