Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
[gcc] Migrate recipe to conan v2, add gfortran to compilation #13896
[gcc] Migrate recipe to conan v2, add gfortran to compilation #13896
Changes from all commits
813cba0
0f55f71
16e70dc
f3ab7a7
17f314b
f4abaca
f71a0a1
be6e640
bcd5dd4
8c600b7
f793fe6
f0489c3
2296651
9d46907
1c22955
d59afa6
924e331
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider adding package_type as recommended in Conan 2.0 documentation (see https://docs.conan.io/en/2.0/reference/conanfile/attributes.html?highlight=package_type#package-type)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd thought about this, but I think that there's also utility in exporting the libraries as well. Especially as it relates to
gfortran
,libgfortran
is often required at link time for any generated executable, so I'm not sure it's feasible for this to be considered only anapplication
package.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Broken how? Which tool is required? Need to consider user configuration too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the validation in the
binutils
recipe:conan-center-index/recipes/binutils/all/conanfile.py
Lines 97 to 98 in 668ee5b
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm unclear on what is driving the need to specify binutils as a build-time dependency for Linux. I was able to build fine without it. If there are conditions where it is needed, it would be nice to scope the dependency down to those circumstances. Also, the latest version of Conan 2.0 doesn't like the use of self.info.settings in the build_requirements method, so while my recommendation would be to eliminate this if-block altogether, at a minimum it likely needs to be changed to:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You were able to build without it because there will be autodiscovery of the your system binutils, which should exist on all linux systems. This is here in the interest of explicitly modelling dependencies and moving away from a reliance on/contamination by your system executables. Specifically, binutils provides the assembler and linker for linux systems. On macos, the assembler and linker are provided by cctools (#14169). I think on windows these are bundled with the msys/mingw subsystems.
This allows a user to specify/override the version of binutils being used explicitly to control the binaries used independently of what's installed on your system.