Related Linear issue here. This is part of a set of diffs that will allow ShellCheck to be added to the CI. See inline comments for specific details of the ShellCheck error/warning output.
Details
Details
This script is run in the tunnelbroker Docker build, so if that CI continues to build with no errors we can reasonably assume this script continues to work as expected.
Also, I ran git grep 'CXXFLAGS' and found no occurrences of it in the codebase.
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
| services/tunnelbroker/docker/install_cryptopp.sh | ||
|---|---|---|
| 9 ↗ | (On Diff #15319) | |
| services/tunnelbroker/docker/install_cryptopp.sh | ||
|---|---|---|
| 9 ↗ | (On Diff #15319) | This was most likely meant to use here $ cat $(nix-build '<nixpkgs>' -A cryptopp.src)/GNUmakefile | grep CXXFLAGS | head -n 8 MACHINEX := $(shell $(CXX) $(CXXFLAGS) -dumpmachine 2>/dev/null) SYSTEMX := $(shell $(CXX) $(CXXFLAGS) -dumpmachine 2>/dev/null) ifeq ($(findstring -DCRYPTOPP_DISABLE_ASM,$(CXXFLAGS)),-DCRYPTOPP_DISABLE_ASM) TCXXFLAGS := $(filter-out -D_FORTIFY_SOURCE=% -M -MM -Wall -Wextra -Werror% -Wunused -Wconversion -Wp%, $(CPPFLAGS) $(CXXFLAGS)) ifneq ($(strip $(TCXXFLAGS)),) $(info Using testing flags: $(TCXXFLAGS)) TCOMMAND = $(CXX) $(TCXXFLAGS) $(TEXTRA) $(ZOPT) $(TOPT) $(TPROG) -o $(TOUT) # Base CXXFLAGS used if the user did not specify them |
| services/tunnelbroker/docker/install_cryptopp.sh | ||
|---|---|---|
| 9 ↗ | (On Diff #15319) | Thank you! That makes sense to me, will update. Also, next time you provide feedback can you select Request Changes so it pops up in my queue/updates for other reviewers? |
Comment Actions
CXXFLAGS is picked up by make
CXXFLAGS Extra flags to give to the C++ compiler.
(https://www.gnu.org/software/make/manual/make.html#index-CFLAGS)
Jon's suggestion to define CXXFLAGS for the make command makes sense
Comment Actions
Also, next time you provide feedback can you select Request Changes so it pops up in my queue/updates for other reviewers?
Keep forgetting this is default behavior for github but not phabricator
Comment Actions
Off topic, but I wonder if you could set up a Herald view to emulate the GitHub behavior
Comment Actions
Minor note, but I'm used to setting the ENV variables before the command... from man bash:
The environment for any simple command or function may be augmented temporarily by prefixing it with parameter assignments, as described above in PARAMETERS. These assignment statements affect only the envi- ronment seen by that command.
which I think would be something like
CXXFLAGS="-DNDEBUG -g2 -O3 -std=c++11" make
Are we sure these flags aren't also relevant for the make libcryptopp.pc and make install commands?
Maybe we should export CXXFLAGS or suppress the shellcheck warning?
Comment Actions
Maybe we should export CXXFLAGS or suppress the shellcheck warning?
IIRC make doesn't consume from ENV unless you use the ?= or := operator
I cat'd some of the file to show that it doesn't do this, instead it just blindly uses it if declared as a positional flag.
Comment Actions
Ah my mistake, thanks for clarifying and linking to that. Should we still append that to all of the make commands?
Comment Actions
Should we still append that to all of the make commands?
Most good makefiles will set it to something reasonable for the install targets, as you likely want a release version. I would say "defer to upstream unless it's detrimental"