search

PaulStoffregen / teensy_loader_cli

Command line Teensy Loader
http://www.pjrc.com/teensy/loader_cli.html
331 stars 152 forks source link

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

Closed stapelberg closed 3 years ago

stapelberg commented 3 years ago

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

Support for CPPFLAGS was originally added by me in https://github.com/PaulStoffregen/teensy_loader_cli/pull/39, and then removed by @hmaarrfk’s PaulStoffregen/teensy_loader_cli#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 https://github.com/PaulStoffregen/teensy_loader_cli/pull/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 commented 3 years ago

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 commented 3 years ago

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.

hmaarrfk commented 3 years ago

Thank you for the clear explanation

hmaarrfk commented 3 years ago

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 commented 3 years ago

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 :)