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 GCC 10.1.0 #1694

Merged
merged 14 commits into from
May 19, 2022
Merged

- add GCC 10.1.0 #1694

merged 14 commits into from
May 19, 2022

Conversation

SSE4
Copy link
Contributor

@SSE4 SSE4 commented May 21, 2020

Specify library name and version: gcc/10.1.0

  • I've read the guidelines for contributing.
  • I've followed the PEP8 style guides for Python code in the recipes.
  • I've used the latest Conan client version.
  • I've tried at least one configuration locally with the
    conan-center hook activated.

Signed-off-by: SSE4 <[email protected]>
@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@uilianries
Copy link
Member

This is a interesting package, but what's the propose, general usage or integrate with CCI?

@SSE4
Copy link
Contributor Author

SSE4 commented May 21, 2020

@uilianries several goals:

  • use as build requires to install GCC (potentially cross-compiler, but also useful on Mac and other systems) within conan profile
  • some real, complex use-case to test cross-building feature (eventually, first iteration doesn't support cross-building), e.g. be able to cross-compile a cross-compiler as an ultimate goal

@Croydon
Copy link
Contributor

Croydon commented May 21, 2020

Cross-compiling a cross-compiler?

Now my brain hurts

@uilianries
Copy link
Member

uilianries commented May 21, 2020

Cross-compiling a cross-compiler?

Now my brain hurts

Recursive building?

recursive-cat

Copy link
Contributor

@madebr madebr left a comment

Choose a reason for hiding this comment

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

Great recipe @SSE4 !!

Doesn't this recipe require (a minimum version of) binutils?
binutils contains ar, ld, ld.gold and as.
These are programs that are essential for assembling and linking.

Comment on lines +122 to +128
cc = os.path.join(bindir, "gcc-%s" % self.version)
self.output.info("Creating CC env var with : " + cc)
self.env_info.CC = cc

cxx = os.path.join(bindir, "g++-%s" % self.version)
self.output.info("Creating CXX env var with : " + cxx)
self.env_info.CXX = cxx
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want to set CC and CXX here?
Isn't that the role of the profile?

Copy link
Contributor

Choose a reason for hiding this comment

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

I assume that they are needed here, if you are using this recipe as a build_requires then you expect it to populate the environment so it is actually used...

Copy link
Member

Choose a reason for hiding this comment

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

Yes in this case, as you require this specific package, you have to use this compiler. Using a CC and CXX from profile, pointing to another compiler, creates an impredecible behavior

Copy link
Contributor Author

Choose a reason for hiding this comment

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

is there a way to set CC within the profile relative to the build requires?
something like:

[build_requires]
gcc/10.1.0@
[env]
CC=os.path.join(self.deps_cpp_info["gcc"].root_path, "bin", "gcc")
CXX=os.path.join(self.deps_cpp_info["gcc"].root_path, "bin", "g++")

I doubt it's possible right now, so I worry there is no way to achieve the same result without env_info

Copy link
Member

Choose a reason for hiding this comment

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

Nope, but it's a relevant feature.

Copy link
Contributor

@derived-coder derived-coder Aug 5, 2021

Choose a reason for hiding this comment

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

@SSE4
I think this is now possible with using conan 1.38.0, jusing python code in profiles.

@uilianries
What does it mean now, should users of this package put it in a profile or as a build_require step in a conan recipe?

@jgsogo
Copy link
Contributor

jgsogo commented May 21, 2020

\o/

...but we are not going to use it for CCI, we don't want this recipe to be a build_requires of every recipe 😅

but yes, I would love to try the cross-building feature with this recipe, this will be really helpful

@uilianries
Copy link
Member

@SSE4 Did you check the package size? I guess it will require ~1GB.

topics = ("gcc", "gnu", "compiler", "c", "c++")
homepage = "https://gcc.gnu.org/"
url = "https://github.com/conan-io/conan-center-index"
license = " GPL-3.0-only"
Copy link
Contributor

Choose a reason for hiding this comment

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

I have seen this a lot: spaces in front of the license ID. How is this happening? Is this a copy&paste thing from the SPDX list which contains somehow spaces or something similar?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is mostly human error, I've just copy-pasted from the SPDX web-site


def package(self):
autotools = self._configure_autotools()
autotools.install(args=self._make_args)
Copy link
Member

Choose a reason for hiding this comment

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

I would run install-strip instead: https://github.com/docker-library/gcc/blob/master/Dockerfile.template#L125

As you can see, the official GCC docker image uses it. The idea is strip the binaries, so the package size will be much smaller.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

okay, let's add this as well

@conan-center-bot

This comment has been minimized.

@jgsogo
Copy link
Contributor

jgsogo commented May 22, 2020

@uilianries

@SSE4 Did you check the package size? I guess it will require ~1GB.

Not so big. According to Artifactory (the conan_package.tgz file) it is 301.49 MB for the Debug build and 94.25 MB for the Release build.

@SSE4
Copy link
Contributor Author

SSE4 commented May 22, 2020

@madebr binutils will be for sure needed for the cross-building use-case. for the regular build, you already have right binutils, so for the first step, it's not a strong requirement.

@SSE4
Copy link
Contributor Author

SSE4 commented May 22, 2020

@jgsogo yes, definitely, it's not going to be used as a build_requires in CCI recipes, that's not an intention right now.

@madebr
Copy link
Contributor

madebr commented May 22, 2020

@madebr binutils will be for sure needed for the cross-building use-case. for the regular build, you already have right binutils, so for the first step, it's not a strong requirement.

Indeed, native binutils will be available when you build the gcc package yourself, but it won't be if you downloaded the package from bintray on a fresh linux installation.
Same goes for glibc.

@SSE4
Copy link
Contributor Author

SSE4 commented May 25, 2020

@madebr I guess you're right, if install on very empty linux, e.g. embedded system, it will probably fail to run due to the missing glibc and bintuils. I'll create binutils recipe as well, probably once this one is merged, as it's the next step for the cross-building. right now, it's a bit out of scope of the initial recipe version, since we can assume for the majority of non-cross-compiled cases that binutils are already there.

@stale stale bot added the stale label Dec 3, 2021
@stale
Copy link

stale bot commented Jan 3, 2022

This pull request has been automatically closed because it has not had recent activity. Thank you for your contributions.

@stale stale bot closed this Jan 3, 2022
@SSE4 SSE4 reopened this Jan 21, 2022
@stale stale bot removed the stale label Jan 21, 2022
@conan-center-bot

This comment has been minimized.

@stale
Copy link

stale bot commented Feb 21, 2022

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Feb 21, 2022
@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

homepage = "https://gcc.gnu.org"
url = "https://github.com/conan-io/conan-center-index"
license = "GPL-3.0-only"
settings = "os", "compiler", "arch", "build_type"
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not just:
settings = "os", "arch"
?

Copy link
Contributor

Choose a reason for hiding this comment

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

We follow this recommendation: https://github.com/conan-io/conan-center-index/blob/master/docs/packaging_policy.md#settings, sometimes it is useful to know self.settings.compiler inside the recipe, so better to keep the setting and remove it from packageID.

@stale
Copy link

stale bot commented Apr 27, 2022

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Apr 27, 2022
@Croydon
Copy link
Contributor

Croydon commented Apr 27, 2022

Some of the latest comments were that it would be good to have a binutils recipe first. We have that now.

How can we move this forward?

@uilianries
Copy link
Member

I think it just need binutils update. 🤔

@stale stale bot removed the stale label Apr 27, 2022
@madebr
Copy link
Contributor

madebr commented Apr 27, 2022

I still don't think this recipe is correct.
A toolchain consists of:

  • binutils for the assembler/linker/various tools.
  • a library targeting an operating system (most commonly glibc on Linux)
  • a compiler, in this case gcc

If we truly want to create a gcc recipe that can be used as a build requirement without needing any file on the build system,
we need all of the above.
All downloadable toolchains on the interwebs have all 3 things above combined.
So I think we need to have a single recipe that combines binutils/gcc/glibc.
That recipe would only be able to target Linux.
For Windows, we'ld need to add mingw-w64 into the mix.

See the mingw-w64/linux recipe on how it consists of binutils + gmp + mpfr + mingw-w64 + gcc.

@SSE4 SSE4 closed this Apr 30, 2022
@SSE4 SSE4 reopened this Apr 30, 2022
@conan-center-bot

This comment has been minimized.

@conan-center-bot
Copy link
Collaborator

All green in build 6 (8069f33340a48a3d044c7a396bc02ec4862c03e1):

  • gcc/10.2.0@:
    All packages built successfully! (All logs)

Copy link
Contributor

@jgsogo jgsogo left a comment

Choose a reason for hiding this comment

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

IMHO we can move this recipe forward, it can be a starting point to improve it. We know there are some really hard recipes that are not perfect, and they might suffer breaking (really breaking) changes in the future (recipe revisions to the rescue!). But I think it is better to start having something and work on top of it.

More people will start to contribute to the effort if they see there is something already available.

@uilianries
Copy link
Member

I'm a Linux user (BTW I use Arch), if it builds on Linux, it's work for me and we can improve later, as Javier said

@conan-center-bot conan-center-bot merged commit d8f83f6 into conan-io:master May 19, 2022
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.

10 participants