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

Revert "No need for CPP flags on linux" #66

Merged
merged 1 commit into from
Apr 10, 2021

Conversation

stapelberg
Copy link
Contributor

This pull request reverts #50, which I have noticed just now.

Support for CPPFLAGS was originally added by me in #39, and then removed by @hmaarrfk’s #50 with the description “no need for CPP flags on linux”.

However, that was incorrect. There is a need for CPPFLAGS on Linux, specifically for example for Debian packaging, which passes additional hardening flags, as my original #39 mentioned.

I verified this just now.

With CPPFLAGS, the build output is:

cc -g -O2 -ffile-prefix-map=/tmp/teensy-loader-cli-2.1=. -fstack-protector-strong -Wformat -Werror=format-security -Wdate-time -D_FORTIFY_SOURCE=2 -s -DUSE_LIBUSB -o teensy_loader_cli teensy_loader_cli.c -lusb -Wl,-z,relro -Wl,-z,now

Without CPPFLAGS, the build output is:

cc -g -O2 -ffile-prefix-map=/tmp/teensy-loader-cli-2.1=. -fstack-protector-strong -Wformat -Werror=format-security -s -DUSE_LIBUSB -o teensy_loader_cli teensy_loader_cli.c -lusb -Wl,-z,relro -Wl,-z,now

Note that -D_FORTIFY_SOURCE=2 is missing when CPPFLAGS is not specified.

@hmaarrfk
Copy link
Contributor

I feel like this should be sent as an issue upstream to debian rather than tacking on a flag that isn't meant for the language.

There are certain flags for CPP that aren't compatible with C. I believe that is the issue i was running into on conda-forge. I am happy to double check in a few hours.

Fortify source is optional and debian decided not to include it in their standard C flags. In the build system you should be At liberty to add it. I wouldn't include unrelated build flags just for it.

@stapelberg
Copy link
Contributor Author

It sounds like you think that CPPFLAGS is for C++ (C Plus Plus, CPP).

However, C++’s flag is called CXXFLAGS. The variable CPPFLAGS is for the C Pre Processor (hence CPP) and is meant for the C programming language.

In fact, make’s default rules include CPPFLAGS: https://www.gnu.org/software/make/manual/html_node/Implicit-Variables.html

Thanks for taking a look at what originally broke. I’m curious what the actual issue is.

@PaulStoffregen PaulStoffregen merged commit 9dbbfa3 into PaulStoffregen:master Apr 10, 2021
@hmaarrfk
Copy link
Contributor

Thank you for the clear explanation

@stapelberg stapelberg deleted the revert-50-patch-2 branch April 10, 2021 13:14
@hmaarrfk
Copy link
Contributor

hmaarrfk commented Apr 10, 2021

Following up. It seems that the latest commit version (which includes this patch) is compiling fine on Linux Aarch64 (and now linux 64bit x86) on conda-forge's build system.

I likely removed it in my review of the build flags (for which I opened #52 #59) and I simply misunderstood what CPPFLAGS meant.

@stapelberg
Copy link
Contributor Author

Following up. It seems that the latest commit version (which includes this patch) is compiling fine on Linux Aarch64 (and now linux 64bit x86) on conda-forge's build system.

Thanks for checking!

I’m glad to hear that everything is working as it should now, and we can keep CPPFLAGS as it is now :)

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