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

gn: add initial gn recipe #5433

Merged
merged 12 commits into from
May 10, 2021
Merged

gn: add initial gn recipe #5433

merged 12 commits into from
May 10, 2021

Conversation

madebr
Copy link
Contributor

@madebr madebr commented May 5, 2021

Specify library name and version: gn/cci.20210429

gn is a build system developed by google, used for google produces (chromium)

I've tested this on Linux (gcc + clang) and Windows.
TODO's: Windows + Macos


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

@madebr madebr mentioned this pull request May 5, 2021
4 tasks
@madebr madebr marked this pull request as draft May 5, 2021 00:46
@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.

recipes/gn/all/conanfile.py Outdated Show resolved Hide resolved
@@ -0,0 +1,3 @@
sources:
"cci.20210429":
url: "https://gn.googlesource.com/gn/+archive/6771ce569fb4803dad7a427aa2e2c23e960b917e.tar.gz"
Copy link
Contributor

Choose a reason for hiding this comment

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

Why no sha256?

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 changes all the time.
Some time stamp is probably spoiling the fun.

Copy link
Contributor

Choose a reason for hiding this comment

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

yep, their servers produce tarballs on fly, and sha256 is always different, it's pain to package. if someone can help how to get stable tarball URL for gn, help is welcome.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm wondering if we could mirror this repo on github and have it sync itself automatically using actions.

Copy link
Contributor

Choose a reason for hiding this comment

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

Something like this actions/checkout#24
But it looks like it only works to sync repos on the same domain.

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's completely possible.
But somebody/organization should invest some time setting it up...

self.copy("gn.exe", src=os.path.join(self._source_subfolder, "out"), dst="bin")

def package_info(self):
bin_path = os.path.join(self.package_folder, "bin")
Copy link
Contributor

Choose a reason for hiding this comment

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

Are these lines really necessary? It is my understanding that conan exposes self.cpp_info.binpath to PATH automatically when a package is listed as a build requirement.

Copy link
Contributor Author

@madebr madebr May 5, 2021

Choose a reason for hiding this comment

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

It's not needed when doing crossbuilds (-pr:h hostprofile -pr:b buildprofile).
But when using the single profile approach (when your profile contains os_build or arch_build, I think the PATH environment variable is not automatically set.

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

ericriff
ericriff previously approved these changes May 6, 2021
if self.settings.compiler.cppstd:
tools.check_min_cppstd(self, 17)
else:
if self._minimum_compiler_version_supporting_cxx17:
Copy link
Contributor

Choose a reason for hiding this comment

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

I've seen this pattern over and over, I'm wondering if this should be a conan built in tool.
It would remove boilerplate code and eliminate the need from the recipe creator of knowing which c++ std supports each compiler.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Some util functions would be nice.
But looking at the support matrix at https://en.cppreference.com/w/cpp/compiler_support,
the minimum required compiler version depends on what c++ features a project desires..

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks. One day... 😄

Copy link
Contributor

Choose a reason for hiding this comment

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

The C++ standard is not enough since the STL support changes between some minor versions it's a manual process for many projects

@ericriff
Copy link
Contributor

ericriff commented May 6, 2021

I'm testing this locally to see how it behaves with the crashpad recipe.
I decided to crosscompile it just for the sake of testing and for my aarch64 platform it fails during linking with:

[208/281] LINK gn
FAILED: gn 
aarch64-oe-linux-g++ --sysroot=/home/eric/.conan/data/SDKdev/1.0/eric/stable/package/5ab84d6acfe1f23c4fae0ab88f26e3a396351ac9/sysroots/aarch64-oe-linux -Wl,-O1 -Wl,--hash-style=gnu -Wl,--as-needed --sysroot=home/eric/.conan/data/SDKdev/1.0/eric/stable/package/5ab84d6acfe1f23c4fae0ab88f26e3a396351ac9/sysroots/aarch64-oe-linux -O3 -fdata-sections -ffunction-sections -Wl,--gc-sections -Wl,-strip-all -Wl,--as-needed -static-libstdc++ -pthread -o gn -Wl,--start-group src/gn/gn_main.o base.a gn_lib.a -Wl,--end-group -ldl
home/eric/.conan/data/SDKdev/1.0/eric/stable/package/5ab84d6acfe1f23c4fae0ab88f26e3a396351ac9/sysroots/x86_64-oesdk-linux/usr/libexec/aarch64-oe-linux/gcc/aarch64-oe-linux/8.2.0/real-ld: cannot find -lstdc++
collect2: error: ld returned 1 exit status

Looking into my sysroots it looks like I only have the shared version of libstdc++:

find -name \*stdc\*
./usr/lib/libstdc++.so.6.0.25-gdb.py
./usr/lib/libstdc++.so.6.0.25
./usr/lib/libstdc++.so.6
./usr/lib/libstdc++.so
./usr/lib/.debug/libstdc++.so.6.0.25
./usr/include/curl/stdcheaders.h
./usr/include/c++/8.2.0/aarch64-oe-linux/bits/stdc++.h
./usr/include/readline/rlstdc.h
./usr/include/stdc-predef.h

There is a --no-static-libstdc++ option for gen.py that makes it link correctly. I'm wondering if we should add it as a conan option.

@SSE4 SSE4 requested a review from uilianries May 6, 2021 08:00
@madebr
Copy link
Contributor Author

madebr commented May 6, 2021

There is a --no-static-libstdc++ option for gen.py that makes it link correctly. I'm wondering if we should add it as a conan option.

On my Fedora Linux box, the static library is provided by libstdc++-static.
Probably, there exists a similar package for your arm system.

Linking to the static library makes the executable more robust/workable on more machines.
gn consists of a single executable, so we don't care about other libraries/executables linking to it.

When using the shared libstdc++, then we cannot remove self.info.settings.compiler in package_id.
For gcc/clang, the version of the standard library depends on the compiler version. (cfr the latest c++ weekly video about breaking ABI)

@conan-center-bot

This comment has been minimized.

@madebr madebr marked this pull request as ready for review May 6, 2021 13:54
recipes/gn/all/conanfile.py Outdated Show resolved Hide resolved
@madebr
Copy link
Contributor Author

madebr commented May 7, 2021

@SpaceIm
Can I get an initial review please?

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

]
if self.settings.build_type == "Debug":
conf_args.append("-d")
self.run("python build/gen.py {}".format(" ".join(conf_args)), run_environment=True)
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it require cpython in build requirement when packaged? Maybe add a TODO?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I'll add a todo.

def build(self):
with tools.chdir(self._source_subfolder):
with self._build_context():
tools.save(os.path.join("src", "gn", "last_commit_position.h"),
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add a comment about what this is for?

self.build_requires("ninja/1.10.2")

@contextmanager
def _build_context(self):
Copy link
Contributor

Choose a reason for hiding this comment

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

Could this not be re-used some how, I hate duplication like this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These environment variables are closely coupled with the .gn sources in the test package.
This is my first encounter with gn, so I don't really know a better way to let the build system know of another compiler.

@conan-center-bot

This comment has been minimized.

@conan-center-bot
Copy link
Collaborator

All green in build 12 (f0988c29e2e8127af3e631eaad85e13946463f23):

  • gn/cci.20210429@:
    All packages built successfully! (All logs)

@conan-center-bot conan-center-bot merged commit b5a9d42 into conan-io:master May 10, 2021
@madebr madebr deleted the gn_recipe branch May 10, 2021 20:51
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.

8 participants