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

configure: fix non-portable "==" tests #218

Merged
merged 2 commits into from
Nov 12, 2022
Merged

Conversation

CyberTailor
Copy link
Contributor

Copy link
Member

@sjaeckel sjaeckel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Basically LGTM!

Would it maybe make sense to add a CI Job which uses dash as default shell, so we can detect stuff like this earlier?

@NexAdn
Copy link

NexAdn commented Nov 5, 2022

Would it maybe make sense to add a CI Job which uses dash as default shell, so we can detect stuff like this earlier?

Maybe there is a linter which can check for bashisms in scripts that should be POSIX-compatible? If not, It should be pretty easy to add another matrix variable which just iterates over bash and dash as interpreter

@CyberTailor
Copy link
Contributor Author

There's checkbashisms but it gives false positives.

So I went with dash -n.

@sjaeckel
Copy link
Member

sjaeckel commented Nov 7, 2022

I like that approach 👍 and TBH I prefer it over some script

Just to be sure I added the revert of 7352bd5 to check whether the added test does its job... and apparently it doesn't do what it should :D

I didn't get dash -n ./configure to break ... don't ask me why

In the end I got it to break locally, but doing so required to uninstall bash ... why? I'm not sure... maybe something inside autotools prefers using bash if it can find it!?

The easiest way I found would be to run configure under dash, can you please fixup your commit be01be5 with a1ddc31 and force-push?

@sjaeckel sjaeckel force-pushed the master branch 2 times, most recently from bc1bc22 to f4f6fbc Compare November 12, 2022 11:57
@sjaeckel sjaeckel force-pushed the master branch 2 times, most recently from c9f3cf6 to e267eba Compare November 12, 2022 12:04
@sjaeckel sjaeckel force-pushed the master branch 2 times, most recently from 45eda2b to 96ece6f Compare November 12, 2022 12:25
@sjaeckel
Copy link
Member

CI Run https://github.com/strophe/libstrophe/actions/runs/3450924402/jobs/5759814792 shows that it works now as intended!

Thanks @CyberTailor for reporting this

@sjaeckel sjaeckel merged commit 96ece6f into strophe:master Nov 12, 2022
@sjaeckel sjaeckel added this to the next milestone Mar 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants