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

Feat/basic cmake python linux #2

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

maxnbk
Copy link
Collaborator

@maxnbk maxnbk commented Mar 22, 2022

These should be a helpful basis to start from, even if we're effectively blocking off windows support with symlink usage for now.

@maxnbk maxnbk added the enhancement New feature or request label Mar 22, 2022
recipes/cmake/package.py Outdated Show resolved Hide resolved
env.PATH.prepend("{this.root}/bin")

tests = {
"version_check": 'echo [[ \\"$(cmake --version | head -n 1)\\" -eq \\"cmake version {version}\\" ]]'.format(version=version())
Copy link
Owner

Choose a reason for hiding this comment

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

I would put the equivalent of this directly into version() instead, may as well abort early if there's a mismatch

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not sure I follow.

}

with scope('config') as config:
config.release_packages_path = '${SW_EXTERNAL_RELEASE_ROOT}'
Copy link
Owner

Choose a reason for hiding this comment

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

why have this? User can already provide their own rezconfig.py as per README.md instructions

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think we should go the way of batteries-included with respect to demonstrating multiple package-repository usage.
Ideally, I'd like to achieve something where specifying "external" / "internal" vendor_type automatically changes the release path.

@@ -0,0 +1,130 @@
#!/usr/bin/env python
Copy link
Owner

Choose a reason for hiding this comment

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

So let's think about this as setting up the standard for what most of our build scripts are gonna look like. I think there's opportunity here for removing lots of redundancy. I propose:

  • we add our own build_utils package that gets built before anything else. Simple, pure python, no deps.
  • We put reusable stuff like BuildConfig class in here
  • having formatted cmake code embedded into py code isn't that easy to read. What if we had that code stored in a CMakeLists.txt.j2 file, and used Jinja2? I know that introduces another build-time dep, but damn if this wouldn't look a bit like ansible, and would help keep things pretty clean.

Copy link
Collaborator Author

@maxnbk maxnbk Apr 23, 2022

Choose a reason for hiding this comment

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

I agree completely, however:
getting cmake and python built are two builds where some redundancy can be permitted because we're going to rely on them to build all the other packages, so we get a little bit chicken-egg-y. It depends on whether we want to allow the system-python-interpreter to be used as a bootstrapping dependency. What are your feelings there?

regarding templatizing cmakelists, I don't yet really see the value, but it's something we can move towards. For now, I'd like to target linux-build-ability, while we consider the best to provide multi-platform / multi-arch / multi-OS support, because we have some logistics issues there. For example, I think the best thing to do WRT to "platform-specific build flags" is to externalize the config from the build and load according to some dict-merge type of system, but what should that look like? I'm a little worried it'll end up looking like qt's 'mkspecs'

even more important is somehow expressing to the package.py which platforms/archs/os's it supports, so that a rez-install actually works when a given platform config is given.


build_requires_source = True
build_requires_internet = False
build_requires_sudo = False
Copy link
Owner

Choose a reason for hiding this comment

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

might wanna drop these from the installed pkg def, prefix with __ to do that, as per https://github.com/nerdvegas/rez/wiki/Package-Definition-Guide#package-attributes

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'd think people might want to rez-search these attributes in the future in their installed packages, so as to be able to identify builds that use sudo, for example, and try to fix them to make them not require it.

recipes/python/build.py Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants