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

Use tests instead of ls to check for file existence #789

Merged
merged 1 commit into from
Apr 22, 2016
Merged

Use tests instead of ls to check for file existence #789

merged 1 commit into from
Apr 22, 2016

Conversation

mmb
Copy link
Contributor

@mmb mmb commented Mar 30, 2016

No description provided.

@dcid
Copy link

dcid commented Mar 30, 2016

That's not portable outside of bash, unfortunately.

@mmb
Copy link
Contributor Author

mmb commented Mar 30, 2016

If the target is Bourne shell those should work (see https://en.wikibooks.org/wiki/Bourne_Shell_Scripting/Appendix_C:_Quick_Reference#File_tests)

Actually it looks like -e only works in bash, but that can be replaced with -f. That script already uses the -f test here: https://github.com/ossec/ossec-hids/blob/master/install.sh#L1227

@dcid
Copy link

dcid commented Apr 11, 2016

I would actually change that other one to get them all in sync using ls :)

But the main question is: what issue are you trying to solve? The current format is very portable and works with bash and old sh. That's why we chose this way to start with.

*note that it might be ok to change, but I don't have access to solaris/hpux/aix, like I used before, to test. These along with some *BSD's were the problem and forced us to use ls for portability.

@ddpbsd
Copy link
Member

ddpbsd commented Apr 11, 2016

It looks like your tree is a bit behind MASTER. Could you update (hoping to get the tests to pass)?

@mmb
Copy link
Contributor Author

mmb commented Apr 13, 2016

I rebased against master and the test went green.

@ddpbsd
Copy link
Member

ddpbsd commented Apr 20, 2016

Anyone with one of these niche systems want to give this a spin?

@ddpbsd
Copy link
Member

ddpbsd commented Apr 22, 2016

I have a rant about AIX/HPUX institutions, but it's not appropriate here. Since there are no reports of this breaking anything (and I doubt there will be until after a release), I'm merging this. Thanks for the PR @mmb, and I expect to hear an "I told you so" @dcid.

@ddpbsd ddpbsd merged commit 910e40c into ossec:master Apr 22, 2016
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.

3 participants