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

Add Toolchain Class for GnuMake #7430

Merged
merged 9 commits into from Aug 30, 2020

Conversation

solvingj
Copy link
Contributor

@solvingj solvingj commented Jul 27, 2020

Changelog: Feature: Add an experimental toolchain for gnu make.
Docs: conan-io/docs#1808

Progresses #7280.

  • 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.

Note: By default this PR will skip the slower tests and will use a limited set of python versions. Check here how to increase the testing level by writing some tags in the current PR body text.

Copy link
Member

@memsharded memsharded left a comment

Choose a reason for hiding this comment

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

Looking very good, keep going. I am fine if the first iteration is basically this, it doesn't need to go much further to be included.

conans/test/functional/toolchain/test_make.py Outdated Show resolved Hide resolved
conans/test/functional/toolchain/test_make.py Outdated Show resolved Hide resolved
@memsharded
Copy link
Member

Did some minor changes in #7435 that will affect this PR, please update when it is merged.

@memsharded memsharded added this to the 1.29 milestone Aug 3, 2020
conans/client/toolchain/make.py Outdated Show resolved Hide resolved
@solvingj
Copy link
Contributor Author

solvingj commented Aug 5, 2020

@memsharded I've added proper handling to a bunch of important flags. Please review the diff of the latest commit if you can to get a feel for it. Let me know if you see any glaring issue.

@solvingj
Copy link
Contributor Author

solvingj commented Aug 7, 2020

Just found issues, this is not ready yet.

… build exe,shared,static binaries. Refactor test structure to be more like generator test.
@solvingj solvingj marked this pull request as ready for review August 7, 2020 19:49
@solvingj
Copy link
Contributor Author

solvingj commented Aug 7, 2020

Recent test issues resolved. Taking out of draft status for CI to run.

@solvingj
Copy link
Contributor Author

solvingj commented Aug 7, 2020

For future reference, this toolchain features a Make function called CONAN_TC_SETUP which works much like CONAN_BASIC_SETUP from the CMake generator which most people are familiar with. It is now also similar to CONAN_BASIC_SETUP in the Make generator which is pending in the following PR:
#7459

These Make functions automatically appends CONAN_ prefixed variables to the standard Make variables. For users who don't want this behavior, they can pick and choose how to use the individual CONAN_ variables manually in any way they wish.

@solvingj solvingj changed the title add rough draft of toolchain class for make Add Toolchain Class for GnuMake Aug 8, 2020
@solvingj
Copy link
Contributor Author

solvingj commented Aug 8, 2020

Added PR for docs: conan-io/docs#1808

@memsharded memsharded requested a review from jgsogo August 25, 2020 08:52
Copy link
Member

@memsharded memsharded left a comment

Choose a reason for hiding this comment

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

This is looking good, but I would try to have the generated toolchain files simpler. Users when reading those should be very easy and clear what the toolchain is doing, without really needing to mentally evaluate and understand a lot of conditionals that will evaluate always to the same thing.

What do you think @solvingj?

@@ -1 +1,2 @@

from .make import MakeToolchain
Copy link
Member

Choose a reason for hiding this comment

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

Is this import necessary? We don't use this in the codebase

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seemed it was necessary at the time. I will try to remove and see if it still works.

conans/client/toolchain/make.py Show resolved Hide resolved
CONAN_TC_DEFINES += NDEBUG
endif

ifeq ($(CONAN_TC_SET_LIBCXX),True)
Copy link
Member

Choose a reason for hiding this comment

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

I think it is better to simplify this, by adding the condition in the template:

  • Do not need to define a CONAN_TC_SET_LIBCXX var.
  • Use set_libcxx as a template condition to render or not certain parts of the template.
  • The idea is that if not setting libcxx, then absolutely nothing will appear in the toolchain file, and it will be simpler.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As discussed earlier I copied most of the patterns from the CMake toolchain for this first iteration.

endif
else ifeq ($(CONAN_TC_COMPILER),sun-cc)
ifeq ($(CONAN_TC_LIBCXX),libCstd)
CONAN_TC_CXXFLAGS += -library=Cstd++
Copy link
Member

Choose a reason for hiding this comment

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

Something similar to the mapping from the CONAN_TC_LIBCXX to the final flag to be passed. It is likely that this is already somewhere in python. So maybe the final toolchain file could be something like just a single line defining the -library=xxx value, but without conditions at all?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same answer as above. Copied patterns from Cmake. Indeed, if we can release this first iteration, we can refactor the common code out together as one concerted initiative.

@memsharded
Copy link
Member

These Make functions automatically appends CONAN_ prefixed variables to the standard Make variables. For users who don't want this behavior, they can pick and choose how to use the individual CONAN_ variables manually in any way they wish.

The idea is that this will be definitely possible, but controlled by the recipe toolchain, not at the build system level. Recipe toolchain defines if the vars are automatically injected or not, if certain behaviors (fpic, libcxx) are used or not, etc.

My feeling is that it is easier to control behavior, to introduce opt-ins/outs and fixes at the python toolchain() level than in the build system .mak files. It is also more transparent, e.g. if the toolchain() defines that fPIC is used, there shouldn't be an easy way to not use it at the build system level, otherwise there will be inconsistencies that are difficult to see and understand by package recipe creators and maintainers. Let's discuss it.

@solvingj
Copy link
Contributor Author

solvingj commented Aug 27, 2020

The idea is that this will be definitely possible, but controlled by the recipe toolchain, not at the build system level. Recipe toolchain defines if the vars are automatically injected or not, if certain behaviors (fpic, libcxx) are used or not, etc.

Per @memsharded discussion on slack, we have a lot of feedback on the pattern of requiring a call to a macro in the build system to activate something in the files conan generated for the build system. The feedback is strong that transparency is most desirable, and calling the function explicitly is unacceptable in many cases.

I understand and recognize that feedback. So, I think the ability to make things transparent should always be a top priority. However, I think there should always be a reasonable way to opt-out of the full transparency, so I think the macros should always be available as an option.

@memsharded suggestion to add a toolchain option which sets the default activation status in the generated file makes sense to me, and I agree that in most OSS recipes, this would probably be True. Conversely, in private enterprise codebases, the implementation of Conan is often phased and gradual. In these cases, I can see many enterprises setting the this to False for private components so they can start generating and committing conan toolchain files to SCM using CI. Then, updating all their existing Makefiles to include these toolchain files across the codebase but without actually affecting builds at all. Once that is done, whomever is responsible for implementing Conan could go project-by-project and manually add the call to the macro for local testing. This lets them easily compare the flags or binaries built with and without the Conan toolchain variables and troubleshooting the difference. Once all the projects successfully build with Conan variables, and the time is appropriate, they could change all the recipes to begin generating and committing new toolchain files with default activation status to true, and thus making the use of Conan toolchains active across the codebase. The point of this very specific "scenario" is only to justify that the macro is still a good thing for phased migrations, and the key moving forward is finding the right control points (and I like the one suggested)

@memsharded
Copy link
Member

As discussed, even if there are some things to change and improve, lets merge and release this as a first iteration, and start gathering feedback to keep this new experimental feature moving forward.

@memsharded memsharded merged commit 951f45a into conan-io:develop Aug 30, 2020
2 checks passed
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

3 participants