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

scripts: prevent globbing and word splitting and other improvements #473

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ljmf00
Copy link
Member

@ljmf00 ljmf00 commented Oct 22, 2020

Signed-off-by: Luís Ferreira [email protected]


I'm very unhappy with the state of this scripts. Most of the things fixed here prevent a lot of word splitting problems.

I would suggest to add a required step to the CI to run shellcheck or some linter. There's codacy which is a free service for open source projects. For shellcheck it's good enough but you need to enable the most important patterns there if you want a decent effect.

Copy link
Member

@wilzbach wilzbach left a comment

Choose a reason for hiding this comment

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

Thanks a lot for your PR. We already use shellcheck for the install script. The other scripts are very old and I am not even sure whether it's a good idea to touch them as building the release binaries for Linux is rather complicated and not tested by the CIs.

script/install.sh Outdated Show resolved Hide resolved
@ljmf00
Copy link
Member Author

ljmf00 commented Oct 22, 2020

Thanks a lot for your PR. We already use shellcheck for the install script. The other scripts are very old and I am not even sure whether it's a good idea to touch them as building the release binaries for Linux is rather complicated and not tested by the CIs.

Well, I left this as a draft, but do you think this legacy scripts should be left unmaintained? My point was to slowly change some things, especially somethings like chmod -R 0755 $TEMPDIR/$UNZIPDIR/*. For people that will try to use this script inside a folder with spaces, it won't work properly.

In my honest opinion, even though this is unmaintained, at least, this should minimally work on some different environments.

@ljmf00
Copy link
Member Author

ljmf00 commented Oct 22, 2020

There's some things that I want to tackle on my contribution to this repo, which is replacing which command to a bash builtin and POSIX compliant code, replace some coreutils-based expressions with bash builtin commands.

@ljmf00 ljmf00 force-pushed the fix-bash-common-errors branch from 2d129a5 to c1cd5a5 Compare October 22, 2020 11:02
@ljmf00
Copy link
Member Author

ljmf00 commented Oct 22, 2020

Btw, am I missing something here? I'm trying to run tests locally without success:
image

@wilzbach
Copy link
Member

Btw, am I missing something here? I'm trying to run tests locally without success:

The errors are because 2.069 is compiled without PIC and thus can only run on old machines without a hardened kernel.

In my honest opinion, even though this is unmaintained, at least, this should minimally work on some different environments.

It's not intended to run on different environments though. There are way too many platform and distribution-specific bits in the install scripts that they can only be run a specifically configured machine anyhow.

Well, I left this as a draft, but do you think this legacy scripts should be left unmaintained?

Well, we plan to replace them with GitHub actions directly at the DMD repository, but so far no one has had time to do so. My point was that I don't want to mess with these scripts (except for real bugs) as we'll hopefully throw them away soon. Furthermore, my current machine isn't setup to run these scripts which is why I am worried about merging changes to them.

The install.sh script, however, has a decent amount of test scripts (though it could always be better), we have CIs setup for it, I have worked on parts of the script etc. - so I feel a lot more confident merging changes to it.

There's some things that I want to tackle on my contribution to this repo, which is replacing which command to a bash builtin and POSIX compliant code, replace some coreutils-based expressions with bash builtin commands.

Sounds good, though if you have time on your hands I want to point out there are actual features that the install.sh script is missing ;-)
For example, auto-update or verification of the script when doing an update.

@ljmf00
Copy link
Member Author

ljmf00 commented Oct 24, 2020

The errors are because 2.069 is compiled without PIC and thus can only run on old machines without a hardened kernel.

Do you mean, the hardened linux kernel branch? https://github.com/anthraxx/linux-hardened ? Currently I'm running Arch Linux 5.8.12-arch1-1 kernel, but I tried to run this test script both under Alpine Linux and Ubuntu based machines through docker. I assume Alpine is not well supported due to musl and apparently unsupported linker. I tried some tricks like using coreutils-gold and patching alpine to use glibc instead but without success I ended up using the Ubuntu docker. I also tried to run directly on my machine without success.

I'm describing my situation, because I think it would be great to have easy access to the tests locally, at least through a Dockerfile. If you guide me on that I can contribute.

It's not intended to run on different environments though. There are way too many platform and distribution-specific bits in the install scripts that they can only be run a specifically configured machine anyhow.

[...]

Well, we plan to replace them with GitHub actions directly at the DMD repository, but so far no one has had time to do so. My point was that I don't want to mess with these scripts (except for real bugs) as we'll hopefully throw them away soon. Furthermore, my current machine isn't setup to run these scripts which is why I am worried about merging changes to them.

The install.sh script, however, has a decent amount of test scripts (though it could always be better), we have CIs setup for it, I have worked on parts of the script etc. - so I feel a lot more confident merging changes to it.

That said, do you prefer not to merge this? I'm completely fine with the decision since this seems not being used and tested, and as you said, will be removed very soon.

Sounds good, though if you have time on your hands I want to point out there are actual features that the install.sh script is missing ;-)
For example, auto-update or verification of the script when doing an update.

Surely. Recently I'm trying to contribute to some D core and community projects. If I've some spare time I definitely going to do it.

@AntonOks
Copy link
Contributor

Sounds good, though if you have time on your hands I want to point out there are actual features that the install.sh script is missing ;-)
For example, auto-update or verification of the script when doing an update.

@ljmf00 The "auto-update" topic of the install.sh script was discussed many times in the past, i.e. see #302 So I guess if you have the time and knowledge, this "feature" would mitigate lot's of complains from the past

I made also a suggestion, which was merged and then reverted by @wilzbach see #457 and #467 Hope this may give you a starting point or idea...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants