nodejs / build

Better build and test infra for Node.
506 stars 166 forks source link

Select compiler broken on AIX #1992

Closed mhdawson closed 5 years ago

mhdawson commented 5 years ago

@rvagg this change seems to have broken select compiler on AIX. https://github.com/nodejs/build/pull/1985

Failure is:

+ rm -rf build
+ git clone https://github.com/nodejs/build.git
Cloning into 'build'...
+ . ./build/jenkins/scripts/select-compiler.sh
/tmp/jenkins4646137741077972430.sh[50]: 0403-057 Syntax error at line 125 : `=~' is not expected.
Build step 'Conditional steps (multiple)' marked build as failure

https://ci.nodejs.org/view/x%20-%20Abi%20stable%20module%20API/job/node-test-node-addon-api-new/nodes=aix61-ppc64/811/console

sam-github commented 5 years ago

The #!/bin/bash on the top of select-compiler.sh is wrong, its not bash, and its not a #! executable, its expected to be sourced. I'll change that to a comment that is correct.

Jenkins on AIX uses /bin/sh explicitly to call select-compiler:

[aix61-ppc64] $ /bin/sh -xe /tmp/jenkins1203817308243684471.sh

So basically select-compiler needs conversion to be /bin/sh syntax.

mhdawson commented 5 years ago

@sam-github unless we can fix this quickly can you back out the previous change? It will block all releases.

sam-github commented 5 years ago

Leaving open til a build passes.

Kicked off: https://ci.nodejs.org/view/x%20-%20Abi%20stable%20module%20API/job/node-test-node-addon-api-new/820/

(I've never run this job before, I limited it to aix61, perhaps someone cat take a look that I did it correctly).

richardlau commented 5 years ago

Leaving open til a build passes.

Kicked off: https://ci.nodejs.org/view/x%20-%20Abi%20stable%20module%20API/job/node-test-node-addon-api-new/820/

(I've never run this job before, I limited it to aix61, perhaps someone cat take a look that I did it correctly).

Unfortunately that build failed while downloading the Node.js binary... probably being affected by https://github.com/nodejs/build/issues/1993.

sam-github commented 5 years ago

Ouch. Well, I'll wait a bit before trying again.

rvagg commented 5 years ago

do you not have Bash on AIX? if you do, then just make sure #!/bin/bash is at the top of the script blocks in Jenkins and it'll all run in Bash and this will be resolved. I already sorted out one instance of bad scripting in node-test-commit-v8-linux yesterday that had #/bin/bash at the top and was running Dash instead, the ! got this sorted. Can we just do the same on AIX?

richardlau commented 5 years ago

Bash does exist on AIX.

rvagg commented 5 years ago
Screen Shot 2019-10-24 at 9 46 17 am

This is node-test-commit-aix, is it in /usr/bin/bash? If so, why was this broken then? That should all have been executed in Bash.

richardlau commented 5 years ago

@rvagg I don't think node-test-commit-aix was broken. This issue references node-test-node-addon-api-new.

sam-github commented 5 years ago

I don't have strong feelings about this, though having been burned in the past porting scripts that assume that all the world is GNU to OSs that are not, I happily stick /bin/sh for scripts, and keep bashisms to interactive use.

The existing script was /bin/sh compatible and worked, now it is /bin/sh compatible again, I don't see the ifelfs as any clearer than the case statements that were used throughout the rest of the file. I'm also not keen to have scripts that depend on magic config typed into jenkins job configs, which are unversioned.

But that's just me, if you really like bash and want to use it everywhere, you're doing the work, its up to you, I've no objections to working code!

And I just saw your post you added.... recall that select-compiler.sh is called from a number of jobs, and they all need the #! added.

rvagg commented 5 years ago

I'm also not keen to have scripts that depend on magic config typed into jenkins job configs, which are unversioned.

That's the world we live in here, we can't escape some dependency on Jenkins, this script depends on being called by Jenkins. It's even invoked in two different ways currently - one by curl and one by git clone, this is "magic" by the same definition. We can't escape this reality unless you want to ditch Jenkins for <some unicorn>. Documentation helps though.

sam-github commented 5 years ago

OK, so would you like me to revert the use of case back to bashisms, or add a commit clarifying that bash syntax is permitted in the future, or shall we just close this as "working for now"?

rvagg commented 5 years ago

Sorry, it's fine as is, my bad for pushing it forward without understanding the implications. I'd prefer us to agree on Bash availability and compatibility for most purposes but it's not a big deal.