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

Remove arbitrary or unnecessary content from make toolchain #7942

Merged

Conversation

solvingj
Copy link
Contributor

@solvingj solvingj commented Oct 26, 2020

From conversation on slack with @memsharded :

at the moment, most of the toolchain is unused, right?

maybe we want to start super minimalistic instead?

something like "things not necessary to have the tests passing".

This implementation removes everything not necessary to have tests passing on linux and windows (mingw32). I think it is minimalistic, but @memsharded please advise if anything else should be changed.

Changelog: Fix: Simplified MakeToolchain to remove things that were not checked by tests or unused.
Docs: omit

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

#TAGS: slow

@solvingj
Copy link
Contributor Author

@memsharded please review (I don't have permission to add a reviewer.)

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.

I am afraid there was some misunderstanding, it seems you went too far.
I meant, all the vars that are not being used at all in the build, like CONAN_TC_ARCH_HOST.

Other vars, they were actually involved in the build, and they were being injected in CONAN_TC_SETUP, right? Then we have at least some evidence that they are not completely broken.

conans/client/toolchain/make.py Outdated Show resolved Hide resolved
conans/client/toolchain/make.py Outdated Show resolved Hide resolved
conans/client/toolchain/make.py Show resolved Hide resolved
@solvingj
Copy link
Contributor Author

I've restored the specific preprocessor definitions requested. Please name or highlight any other specific flags or definitions which you would like restored. I think it's best for you to choose the list explicitly in this case.

@solvingj
Copy link
Contributor Author

solvingj commented Oct 28, 2020

@memsharded Here are four items related to settings which were removed. Please indicate any of these which should be restored.

libcxx_flag 
cppstd_flag
build_type_flags
arch_flag

@memsharded
Copy link
Member

libcxx_flag
cppstd_flag
build_type_flags
arch_flag

These should be restored, making sure they are actually being used in the compilation, checking the binary or introducing architecture or cppstd specific code, it depends.
Maybe it can be delayed until users feedback, to learn actual user needs and patterns. For example, is it expected that Conan defines "Debug" or "Release" CXXFLAGS? Maybe not, that would be part of users Makefiles, right? Maybe it is an opt-in for users that want us to give the utility?

@memsharded memsharded merged commit d27ac17 into conan-io:develop Oct 29, 2020
1 of 2 checks passed
@memsharded
Copy link
Member

I have merged it, lets build step by step on the minimalistic toolchain

@memsharded memsharded added this to the 1.31 milestone Oct 29, 2020
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

2 participants