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/update bx #13866

Merged
merged 21 commits into from
Jan 8, 2023
Merged

Add/update bx #13866

merged 21 commits into from
Jan 8, 2023

Conversation

RazielXYZ
Copy link
Contributor

@RazielXYZ RazielXYZ commented Oct 30, 2022

Specify library name and version: bx/cci.20221022

This is the bx library, a base library used by bimg and bgfx, which are not (yet) on cci. The plan is to bring those over as well.
The previous versions on cci seemed to have used a custom CMake build script. I've opted instead to use the official projrect generator system used upstream, genie, a fork of premake, and then building the generated projects through their suitable conan builders.

Other notes:

  • bimg and bgfx will require the source of a suitable version of bx for their own build process. bgfx also requires the source of bimg.
  • bx and bimg only support static, fPIC builds. bgfx does support shared builds as well.
  • The bx tools are primarily for end-user use and are not required in any builds.
  • The genie project generator tool is provided as a prebuilt binary for all supported platforms in bx's repo, and as such is downloaded in with the sources. This is then ran in the conanfile. If this is not considered okay for cci, we could potentially update and use the genie from cci as a build requirement.

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

@conan-center-bot

This comment has been minimized.

recipes/bx/all/conanfile.py Outdated Show resolved Hide resolved
recipes/bx/all/conanfile.py Outdated Show resolved Hide resolved
@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

Support msvc dynamic runtime
@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@RazielXYZ
Copy link
Contributor Author

Looks like included genie won't run on some CI platforms due to glibc versions, what would be the best way to proceed? Update and use cci genie?

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

Add minimum compiler checks
@conan-center-bot

This comment has been minimized.

Add FreeBSD while we're at it
@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.

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@RazielXYZ
Copy link
Contributor Author

I wouldn't mind some pointers as to how to get mingw working as well; I can see CCI doesn't build it, but trying it in proof of conan showed that msys2 wasn't enough to get x86_64-w64-mingw32-g++ to be present (or on the path?)

Copy link
Contributor

@prince-chrismc prince-chrismc left a comment

Choose a reason for hiding this comment

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

Did a quick pass -- this is really complicated and I will circle back to send a little more time

recipes/bx/all/conanfile.py Outdated Show resolved Hide resolved
recipes/bx/all/conanfile.py Outdated Show resolved Hide resolved
recipes/bx/all/conanfile.py Outdated Show resolved Hide resolved
recipes/bx/all/conanfile.py Outdated Show resolved Hide resolved
@RazielXYZ
Copy link
Contributor Author

RazielXYZ commented Dec 13, 2022

It is indeed quite complicated. bimg and bgfx are going to be pretty much the same - mostly identical, really. So we have that to look forward to as well.😅

Remove unused attributes
Use is_msvc_static_runtime for determining runtime for genie
Get_safe fPIC
Use "src" for src_folder
@conan-center-bot

This comment has been minimized.

Comment on lines 105 to 106
# Save bx path for convencience
self.bx_path = os.path.join(self.source_folder, self._bx_folder)
Copy link
Contributor

Choose a reason for hiding this comment

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

This could be a property

    @property
    def bx_path(self):
        return os.path.join(self.source_folder, self._bx_folder)

This would address the linters complaints

Suggested change
# Save bx path for convencience
self.bx_path = os.path.join(self.source_folder, self._bx_folder)

if version < minimal_version[compiler]:
raise ConanInvalidConfiguration("%s requires a compiler that supports at least C++%s" % (self.name, minimal_cpp_standard))
if self.settings.os == "Windows":
self.lib_ext = "*.lib"
Copy link
Contributor

Choose a reason for hiding this comment

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

All of these are exclusively need in the package() so it would be nice to move these 🙏 they make a lot of noise from the linter in the diff view

cmake = CMake(self)
cmake.configure()
cmake.build()
if can_run(self):
Copy link
Contributor

Choose a reason for hiding this comment

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

This should not be required 🤔 is there a reason this was added?

Copy link
Contributor

@prince-chrismc prince-chrismc 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 being picky about the linters because it super difficult to review this very wacky build system custom scripts.

Honestly I have no Idea 🙃

EDIT: I added this to our weekly meeting to get more input for myself

@RazielXYZ
Copy link
Contributor Author

Think I got rid of all the linter complaints now, thanks for the pointers 🥳

@prince-chrismc
Copy link
Contributor

Thanks for playing along, I'll try to get an answer about the rest so we can keep moving forward :)

@conan-center-bot
Copy link
Collaborator

Conan v1 pipeline

All green in build 19 (bed4710e264101268d5fd0ddff804f317e1b7e3f):

  • bx/cci.20221116@:
    All packages built successfully! (All logs)

@prince-chrismc
Copy link
Contributor

Sorry for taking a while I finally found this email ( I did not write the PR number in my notes ).....

Since this uses a "one off" implementation of a build system, our quality check is the CI is green and the hooks passed.
This generally means the package contents made something normal which however it's not something we have given the. There's nothing obvious but there are not enough expertise to catch anything small.

If this will be added to many recipes... it would be worth making it a proper generator and saving it as a python requires... and we can discuss using it within ConanCenter... current this is not allowed because it might give too much power.

This Genie build system does not seem worth adding to the main client since it pretty rare but it's very interesting to see

@RazielXYZ
Copy link
Contributor Author

At the moment this will also be used for bimg and bgfx, for CCI, if that's fine.
There aren't a lot of other popular or large projects using genie yet, but there is at least one other - SoLoud, which is a great audio library/framework, although development on it has been slow/stopped for a couple years now. Even considering that, it may be worth adding to CCI as well.
There are also a few well-known non-library projects such as MAME and MTuner that use genie as their main build system, but those are likely not relevant to CCI, considering we mostly care about libs and build tools here!

@prince-chrismc
Copy link
Contributor

That's totally fine, if you plan to contribute those recipe (and/or updates) just keep that in the back of the mind.

We'll cross that bridge when we get there. Great work here 😉

@conan-center-bot conan-center-bot merged commit 6f7ac0c into conan-io:master Jan 8, 2023
AbrilRBS pushed a commit to AbrilRBS/conan-center-index that referenced this pull request Jan 16, 2023
* Initial bx (with genie) cci implementation, locally tested

* Attempt to fix mingw and osx

* Fix arch append again

* Requested changes

* Fix some linter warnings

* Delete fPIC on windows
Support msvc dynamic runtime

* Fix msvc runtime check

* Use genie from conan as a tool require instead of the included one
Move layout to where it should be

* Add newest version to config.yml too

* Add lib dl to linux
Add minimum compiler checks

* Fix up compiler support (primarily for clang)
Add FreeBSD while we're at it

* Fix genie compiler call some more

* Fix clang project folder names too

* Add msys2 for mingw builds

* Attempt to fix apple-clang (as it has no special folder on mac due to being the default)

* Fix windows path for msys in mingw build

* Move path fixing up a bit

* Attempt to add required osx/ios system libs (Foundation and Cocoa)

* Add apple frameworks hopefully the right way this time

* Move stuff to snake_case
Remove unused attributes
Use is_msvc_static_runtime for determining runtime for genie
Get_safe fPIC
Use "src" for src_folder

* Change stuff to @Property and move lib per-platform prefix and suffix to package

Co-authored-by: Raziel Alphadios <raziely@gmail.com>
@RazielXYZ RazielXYZ mentioned this pull request Jan 17, 2023
3 tasks
@RazielXYZ RazielXYZ mentioned this pull request Feb 19, 2023
3 tasks
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.

6 participants