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

2023.2 release #185

Merged

Conversation

jfpanisset
Copy link
Contributor

  • Conan builds now use docker cache mounts, no longer get pulled into docker build context, which helps a lot for local builds
  • update docker buildx plugin
  • OpenVDB and OpenEXR build containers get PyBind11
  • use a mirror to download Qt 5.15 faster

- Conan builds now use docker cache mounts, no longer get pulled into
docker build context, which helps a lot for local builds
- update docker buildx plugin
- OpenVDB and OpenEXR build containers get PyBind11

Signed-off-by: Jean-Francois Panisset <[email protected]>
Downloading qt opensource from qt.io will time out CI builds,
use funet.fi mirror instead (was already in Conan recipe)

Signed-off-by: Jean-Francois Panisset <[email protected]>
@jfpanisset jfpanisset marked this pull request as draft November 8, 2023 19:48
Signed-off-by: Jean-Francois Panisset <[email protected]>
@jfpanisset jfpanisset marked this pull request as ready for review November 13, 2023 07:01
@jfpanisset jfpanisset marked this pull request as draft November 13, 2023 07:01
@jfpanisset jfpanisset marked this pull request as ready for review November 13, 2023 07:17
@jfpanisset jfpanisset requested a review from lgritz November 13, 2023 07:18
Copy link
Contributor

@aloysbaillet aloysbaillet left a comment

Choose a reason for hiding this comment

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

Thanks JF! Looks great! I just have a couple of mostly stylistic comments.

Comment on lines +60 to +63
# if version_info.ci_common_version != major_version:
# # Only bake conan images in ci_common container!
# version = version_info.ci_common_version
# major_version = utils.get_major_version(version)
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this whole block be removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I went back and forth on this one several times, but I think now with the "building Conan packages in buildx" support, this is no longer valid, so I'll get rid of the block altogether.

@@ -35,7 +35,7 @@ def test_package_baseqt_2019_dict(self):
qt_version = list(
index.Index().iter_versions(constants.ImageType.PACKAGE, "qt")
)[0]
baked = b.make_bake_dict()
baked = b.make_bake_dict(False, False, False, False)
Copy link
Contributor

Choose a reason for hiding this comment

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

I usually prefer defining default arg value on the function, and specify non-default args when needed using my_arg=True, my_other_arg=True as the False, False, False, False combo isn't very informative

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was trying to go for "minimum footprint" changes and wasn't very clear on how best to propagate these parameters. I'll update the function signature accordingly.

d = self.make_bake_dict()
def make_bake_jsonfile(
self,
keep_source: bool,
Copy link

Choose a reason for hiding this comment

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

You could make this function keyword only to avoid confusion on what does the Boolean does. You can do it by putting a * as argument before keep_source. This with default values makes it easier to use and less error prone

@jfpanisset
Copy link
Contributor Author

I will merge as is so I can make some releases and address the comments in a follow up branch, thank you very much!

@jfpanisset jfpanisset merged commit d6f74a5 into AcademySoftwareFoundation:master Nov 21, 2023
11 checks passed
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.

3 participants