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

correct/improve/simplify makefile unit test #4018

Merged
merged 4 commits into from Dec 3, 2018

Conversation

Projects
None yet
2 participants
@solvingj
Copy link
Contributor

commented Nov 29, 2018

Changelog: Fix: Improve make generator test

This is a followup adjustment to:
#4003
which closed:
#3773

The makefile generator unit test still had some small issues, summarized as they appear below. On one hand, the builds and tests passed, making these seem like mostly cosmetic issues. On the other hand, people sometimes look at tests, and probably moreso for new generators than anything else. Non-gcc experts might be confused about how they should use these flags/vars in their own makefiles, and gcc-experts might be concerned that the unit test does things incorrectly and still passes. Best to make these test as correct as possible.

-fPIC/-fPIE are particularly important flags, which need to go to both compiler and linker, and should always be set by the current package, never propagated from upstream dependencies.

-march/-mtune would be defined on "platform" basis, likely passed in via a profile/environment_varaible/toolchain file rather than from upstream dependencies.

-Thus, replaced the cflags above with ones that people might realistically want to pass from upstream to downstream. Of note, -pthread is indeed a compiler flag separate from the linker flag -lpthread.

-Modified CXX_INCLUDE_PATHS var name to be consistent with CONAN_INCLUDE_PATHS

-Added new CFLAGS variable and set it equal to $(CONAN_CFLAGS). It turned out that the latter was simply ignored (never referenced) in the build, and that was definitely a problem. This is probably the most significant of the improvements.

-Simply removed the use of the CXX_FLAGS variable. There was no reason to add a new variable, as it just complicates the example overall. Also, this removes the -fPIC compiler flag from the equation completely, which is good because it's not strictly necessary. To handle fPIC and fPIE properly, one should make it a Conan option and only apply it conditionally in the makefile, none of which is necessary for this test.

  • Refer to the issue that supports this Pull Request.
  • If the issue has missing info, explain the purpose/use case/pain/need that covers this Pull Request.
  • I've read the Contributing guide.
  • I've followed the PEP8 style guides for Python code.
  • I've opened another PR in the Conan docs repo to the develop branch, documenting this one.

solvingj added some commits Nov 29, 2018

add proper use of CONAN_EXELINKFLAGS and CONAN_SHAREDLINKFLAGS, also,…
… pass CFLAGS to linker rather than CPPFLAGS, eliminate superfluous LIBS var

@danimtb danimtb self-assigned this Nov 30, 2018

@danimtb

This comment has been minimized.

Copy link
Member

commented Nov 30, 2018

Thanks @solvingj for the explanation. I think having a more realistic test is always a good thing.

Having a look at the variables I fail to see the same logic as described here: https://docs.conan.io/en/latest/reference/build_helpers/autotools.html#environment-variables

Which I think is the right convention as I explained with the table in here: conan-io/docs#955 (comment)

We need to reach an agreement or probably look more into it

@danimtb

This comment has been minimized.

Copy link
Member

commented Nov 30, 2018

Also note that the hellowrapper recipe requires a hello one compiled with CMake that should be setting automatically the fPIC flag and the test in CI is failing

@danimtb

danimtb approved these changes Dec 3, 2018

@danimtb danimtb added this to the 1.10 milestone Dec 3, 2018

@danimtb danimtb merged commit 8b25a26 into conan-io:develop Dec 3, 2018

2 checks passed

continuous-integration/jenkins/pr-head This commit looks good
Details
license/cla Contributor License Agreement is signed.
Details

@ghost ghost removed the stage: queue label Dec 3, 2018

grisumbras pushed a commit to grisumbras/conan that referenced this pull request Dec 27, 2018

correct/improve/simplify makefile unit test (conan-io#4018)
* correct/improve/simplify makefile unit test

* add proper use of CONAN_EXELINKFLAGS and CONAN_SHAREDLINKFLAGS, also, pass CFLAGS to linker rather than CPPFLAGS, eliminate superfluous LIBS var

* map CONAN_CPPFLAGS to CXXFLAGS and implement fPIC and fPIE in test makefiles, remove typeo

* fix LDLIBS var names and add to commands, remove CFLAGS from commands
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.