Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

builds are broken on systems using dash as /bin/sh #10510

Closed
ryao opened this issue Jun 28, 2020 · 5 comments
Closed

builds are broken on systems using dash as /bin/sh #10510

ryao opened this issue Jun 28, 2020 · 5 comments
Labels
Type: Building Indicates an issue related to building binaries

Comments

@ryao
Copy link
Contributor

ryao commented Jun 28, 2020

We received a bug report at Gentoo regarding this:

https://bugs.gentoo.org/724452

Debian based systems use dash as /bin/sh by default and Gentoo offers it as an option. Builds on such systems were broken by ff3e2e3, but there were commits slowly breaking things well before then, such as 85ce3f4. The checkbashisms script can find some issues, although the latter commit simply breaks it. It also misses issues in shell code that gets processed by eval, which is the breakage that prompted the Gentoo bug. The following fixes the issue that got caught in the bug report:

diff --git a/config/kernel.m4 b/config/kernel.m4
index dce619729..2c871549d 100644
--- a/config/kernel.m4
+++ b/config/kernel.m4
@@ -603,7 +603,7 @@ AC_DEFUN([ZFS_LINUX_COMPILE], [
        AC_TRY_COMMAND([
            KBUILD_MODPOST_NOFINAL="$5" KBUILD_MODPOST_WARN="$6"
            make modules -k -j$TEST_JOBS -C $LINUX_OBJ $ARCH_UM
-           M=$PWD/$1 &>$1/build.log])
+           M=$PWD/$1 2>&1 >$1/build.log])
        AS_IF([AC_TRY_COMMAND([$2])], [$3], [$4])
 ])

However, this alone is insufficient to fix the build, as there are other bashisms in the code. One of us (probably me) needs to sit down and comb through the autotools code to find and squash the various bashisms. I am filing this issue in advance of a more complete patch now that the issue has been properly triaged.

@ryao
Copy link
Contributor Author

ryao commented Jun 28, 2020

About half of the issues flagged by checkbashisms are the following:

https://unix.stackexchange.com/questions/57840/posix-test-and-a

That is not an actual bashism, but it is simultaneously non-portable and a pain to read, with the alternative being much more readable.

@behlendorf Do you have any objections to dropping -a and -o in favor of && and ||?

Also, we probably should create a test case using checkbashisms to minimize the possibility of future regressions once this is fixed. I am not sure how adding that to our tests would work given that we would need to install the script on the various buildbots.

@ghfields
Copy link
Contributor

I'll admit, I've burned reviewer time identifying and eliminating bashisms in my proposed code. In my case, it got worked out, but including the checkbashisms in the test suite to would reduce reviewer burden and speedup code improvement suggestions.

@behlendorf behlendorf added the Type: Building Indicates an issue related to building binaries label Jun 29, 2020
@behlendorf
Copy link
Contributor

@ryao no I don't have any objection to dropping -a and -o in favor of && and ||. Rather than creating a test case for this I'd suggest we add a new builder to the CI which verifies the build in a /bin/sh environment. Similar to what we already do for the non-x86 architectures to avoid breaking those builds. Can you suggest such a setup that would be easy to boot and run in ec2? I thought we were already testing with dash on Ubuntu, but bash may getting pulled in accidentally as a dependency of something else.

@gyakovlev
Copy link
Contributor

I worked around it in gentoo by using setting CONFIG_SHELL="/bin/bash" before ,/configure runs for 0.8..
2.0.0_rc
seems to be unaffected by this or maybe affected differently. I'll also try to comb the code a bit once I have time.

@behlendorf
Copy link
Contributor

Actually, I believe we can close this one out. There was a lot of work done to the build system and support scripts to get rid of these kind of bashisms. We've also enabled checkbashisms as part of our normal linting to try and keep them out. If you do run in to issues with 2.0.0-rc2 let me know and we can reopen this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Building Indicates an issue related to building binaries
Projects
None yet
Development

No branches or pull requests

4 participants