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 type cmp to test for cmp in Makefile #869

Merged
merged 1 commit into from Jan 27, 2017
Merged

Use type cmp to test for cmp in Makefile #869

merged 1 commit into from Jan 27, 2017

Conversation

dylanjgscott
Copy link
Contributor

@dylanjgscott dylanjgscott commented Nov 8, 2016

This PR will allow building on BSD based systems.

The --version option to cmp doesn't exist in BSD so cmp --version always returns non-zero status on BSD systems, causing make to fail.

Using type cmp instead of cmp --version will work on both GNU and BSD systems.

@dylanjgscott dylanjgscott reopened this Nov 9, 2016
Copy link
Contributor

@cbbrowne cbbrowne left a comment

Choose a reason for hiding this comment

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

Seems like a good change, with the caveat that I'm not utterly certain "type" works everywhere. Usually Linux uses Bash, but Debian and Ubuntu often are using Dash, a POSIX-compatible smaller shell

Note https://en.wikipedia.org/wiki/Type_%28Unix%29

It's also possible that a good command, per POSIX, would be "command -V cmp".

cbbrowne@cbbrowne ~/G/q/k/p/k/cbbrowne> command -V cmp
cmp is /usr/bin/cmp
cbbrowne@cbbrowne ~/G/q/k/p/k/cbbrowne> /bin/dash
$ command -V cmp
cmp is a tracked alias for /usr/bin/cmp
$ type cmp
cmp is a tracked alias for /usr/bin/cmp
$ which cmp
/usr/bin/cmp

That suggests that command -V, type, and which are all plausible choices.

@dylanjgscott
Copy link
Contributor Author

dylanjgscott commented Dec 5, 2016

I'm not certain that type will work absolutely everywhere either but it will at least work on Mac, GNU/Linux and BSD. I would guess that if type cmp doesn't work on any system then cmp --version won't work on that system either.

@fredizzimo
Copy link
Contributor

What about cmp --help would that work on all platforms that has the cmp command?

@dylanjgscott
Copy link
Contributor Author

No, unfortunately BSD doesn't have a --help option.

@fredizzimo
Copy link
Contributor

I guess then we could do cmp $(ROOT_DIR)/Makefile $(ROOT_DIR)/Makefile which would actually test that the cmp command in itself compares two equal files as the same.

@dylanjgscott
Copy link
Contributor Author

Yeah, that's a good idea. I've changed my PR to use cmp $(ROOT_DIR)/Makefile $(ROOT_DIR)/Makefile.

@fredizzimo
Copy link
Contributor

@jackhumbert, This pull request has been open for a long time now, and I believe the implementation should work on all platforms now, so I think this is pretty safe to be merged.

@jackhumbert
Copy link
Member

Thanks for the ping :) Thanks all!

@jackhumbert jackhumbert merged commit a28f689 into qmk:master Jan 27, 2017
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

4 participants