Skip to content

Boost: Use Headers instead of building when not required#2947

Closed
dgovil wants to merge 1 commit intoPixarAnimationStudios:devfrom
dgovil:boost_headers_only
Closed

Boost: Use Headers instead of building when not required#2947
dgovil wants to merge 1 commit intoPixarAnimationStudios:devfrom
dgovil:boost_headers_only

Conversation

@dgovil
Copy link
Copy Markdown
Collaborator

@dgovil dgovil commented Feb 13, 2024

Description of Change(s)

When building USD without Python, OIIO or VDB, Boost libs aren't required. In this scenario, it is preferable to just copy over the headers instead and save some time and complexity.

Edit: as subsequently requested, removed the filtering. With filtering, this can reduce the header size from 180M to 37M. Without filtering, the only benefit is that you don't spend time building with B2 when you don't end up using any of its output.

This also has a significant benefit that it allows the core USD build to be supported on platforms that Boost doesn't natively build on yet like iOS and visionOS. It means that the build_usd.py script doesn't need to carry as many patches to Boost itself if someone is just building the core USD library.

This is similar to PR 2914 but can short circuit the entire bootstrap and b2 process, which may still error on platforms that b2 doesn't understand.

  • I have verified that all unit tests pass with the proposed changes
  • I have submitted a signed Contributor License Agreement

@dgovil dgovil force-pushed the boost_headers_only branch from f61073a to 1145258 Compare February 13, 2024 19:15
@jesschimein
Copy link
Copy Markdown
Collaborator

Filed as internal issue #USD-9296

@dgovil dgovil mentioned this pull request Feb 14, 2024
2 tasks
dgovil added a commit to dgovil/USD that referenced this pull request Feb 14, 2024
…ot include Imaging, and any Imaging related components at this time. Imaging will be added in a follow up PR.

It is a minimal version of PixarAnimationStudios#2455 against the latest `dev` branch.
Changes include:
* Using latest dev branch
* No imaging support. Will be added in a follow up PR.
* Makes use of CMake's inbuilt iOS support, negating the need for toolchain support
* Structures the code in such a way that we can add support for other iOS derived platforms+simulator in future PRs
* Swaps `ARCH_OS_IOS` with `ARCH_OS_IPHONE` to align with compiler directives. IPHONE refers to all derivative platforms, whereas iOS refers to only iPhone/iPad (Confusing but the case for historical reasons as [documented here](https://chaosinmotion.com/2021/08/02/things-to-remember-compiler-conditionals-for-macos-ios-etc/))
* Upgrades MaterialX to 1.38.8 (from 1.38.7) as that adds support for iOS derivative platforms as well.

This PR requires PixarAnimationStudios#2947 to be merged first as that allows for not requiring Boost patches per platform, which can get fragile.
dgovil added a commit to dgovil/USD that referenced this pull request Feb 14, 2024
…ot include Imaging, and any Imaging related components at this time. Imaging will be added in a follow up PR.

It is a minimal version of PixarAnimationStudios#2455 against the latest `dev` branch.
Changes include:
* Using latest dev branch
* No imaging support. Will be added in a follow up PR.
* Makes use of CMake's inbuilt iOS support, negating the need for toolchain support
* Structures the code in such a way that we can add support for other iOS derived platforms+simulator in future PRs
* Swaps `ARCH_OS_IOS` with `ARCH_OS_IPHONE` to align with compiler directives. IPHONE refers to all derivative platforms, whereas iOS refers to only iPhone/iPad (Confusing but the case for historical reasons as [documented here](https://chaosinmotion.com/2021/08/02/things-to-remember-compiler-conditionals-for-macos-ios-etc/))
* Upgrades MaterialX to 1.38.8 (from 1.38.7) as that adds support for iOS derivative platforms as well.

This PR requires PixarAnimationStudios#2947 to be merged first as that allows for not requiring Boost patches per platform, which can get fragile.
dgovil added a commit to dgovil/USD that referenced this pull request Feb 14, 2024
…ot include Imaging, and any Imaging related components at this time. Imaging will be added in a follow up PR.

It is a minimal version of PixarAnimationStudios#2455 against the latest `dev` branch.
Changes include:
* Using latest dev branch
* No imaging support. Will be added in a follow up PR.
* Makes use of CMake's inbuilt iOS support, negating the need for toolchain support
* Structures the code in such a way that we can add support for other iOS derived platforms+simulator in future PRs
* Swaps `ARCH_OS_IOS` with `ARCH_OS_IPHONE` to align with compiler directives. IPHONE refers to all derivative platforms, whereas iOS refers to only iPhone/iPad (Confusing but the case for historical reasons as [documented here](https://chaosinmotion.com/2021/08/02/things-to-remember-compiler-conditionals-for-macos-ios-etc/))
* Upgrades MaterialX to 1.38.8 (from 1.38.7) as that adds support for iOS derivative platforms as well.

This PR requires PixarAnimationStudios#2947 to be merged first as that allows for not requiring Boost patches per platform, which can get fragile.
dgovil added a commit to dgovil/USD that referenced this pull request Feb 14, 2024
…ot include Imaging, and any Imaging related components at this time. Imaging will be added in a follow up PR.

It is a minimal version of PixarAnimationStudios#2455 against the latest `dev` branch.
Changes include:
* Using latest dev branch
* No imaging support. Will be added in a follow up PR.
* Makes use of CMake's inbuilt iOS support, negating the need for toolchain support
* Structures the code in such a way that we can add support for other iOS derived platforms+simulator in future PRs
* Swaps `ARCH_OS_IOS` with `ARCH_OS_IPHONE` to align with compiler directives. IPHONE refers to all derivative platforms, whereas iOS refers to only iPhone/iPad (Confusing but the case for historical reasons as [documented here](https://chaosinmotion.com/2021/08/02/things-to-remember-compiler-conditionals-for-macos-ios-etc/))
* Upgrades MaterialX to 1.38.8 (from 1.38.7) as that adds support for iOS derivative platforms as well.

This PR requires PixarAnimationStudios#2947 to be merged first as that allows for not requiring Boost patches per platform, which can get fragile.
dgovil added a commit to dgovil/USD that referenced this pull request Feb 14, 2024
…ot include Imaging, and any Imaging related components at this time. Imaging will be added in a follow up PR.

It is a minimal version of PixarAnimationStudios#2455 against the latest `dev` branch.
Changes include:
* Using latest dev branch
* No imaging support. Will be added in a follow up PR.
* Makes use of CMake's inbuilt iOS support, negating the need for toolchain support
* Structures the code in such a way that we can add support for other iOS derived platforms+simulator in future PRs
* Swaps `ARCH_OS_IOS` with `ARCH_OS_IPHONE` to align with compiler directives. IPHONE refers to all derivative platforms, whereas iOS refers to only iPhone/iPad (Confusing but the case for historical reasons as [documented here](https://chaosinmotion.com/2021/08/02/things-to-remember-compiler-conditionals-for-macos-ios-etc/))
* Upgrades MaterialX to 1.38.8 (from 1.38.7) as that adds support for iOS derivative platforms as well.

This PR requires PixarAnimationStudios#2947 to be merged first as that allows for not requiring Boost patches per platform, which can get fragile.
@dgovil dgovil force-pushed the boost_headers_only branch 4 times, most recently from 162e736 to b71bf91 Compare February 15, 2024 20:54
@meshula
Copy link
Copy Markdown
Member

meshula commented Feb 15, 2024

Heh nice to see this latest patch reduces the boost footprint in the build directory from ~180MB of headers to ~40MB of headers!

dgovil added a commit to dgovil/USD that referenced this pull request Feb 15, 2024
…ot include Imaging, and any Imaging related components at this time. Imaging will be added in a follow up PR. MaterialX is also disabled as requested and the upgrade will be handled in a follow up PR.

It is a minimal version of PixarAnimationStudios#2455 against the latest `dev` branch.
Changes include:
* Using latest dev branch
* No imaging support. Will be added in a follow up PR.
* Makes use of CMake's inbuilt iOS support, negating the need for toolchain support
* Structures the code in such a way that we can add support for other iOS derived platforms+simulator in future PRs
* Swaps `ARCH_OS_IOS` with `ARCH_OS_IPHONE` to align with compiler directives. IPHONE refers to all derivative platforms, whereas iOS refers to only iPhone/iPad (Confusing but the case for historical reasons as [documented here](https://chaosinmotion.com/2021/08/02/things-to-remember-compiler-conditionals-for-macos-ios-etc/))
* TBB requires SDKROOT to be passed in or it can often go off and find some random compiler toolchain and fail.

This PR requires PixarAnimationStudios#2947 to be merged first as that allows for not requiring Boost patches per platform, which can get fragile.
dgovil added a commit to dgovil/USD that referenced this pull request Feb 19, 2024
…ot include Imaging, and any Imaging related components at this time. Imaging will be added in a follow up PR. MaterialX is also disabled as requested and the upgrade will be handled in a follow up PR.

It is a minimal version of PixarAnimationStudios#2455 against the latest `dev` branch.
Changes include:
* Using latest dev branch
* No imaging support. Will be added in a follow up PR.
* Makes use of CMake's inbuilt iOS support, negating the need for toolchain support
* Structures the code in such a way that we can add support for other iOS derived platforms+simulator in future PRs
* Swaps `ARCH_OS_IOS` with `ARCH_OS_IPHONE` to align with compiler directives. IPHONE refers to all derivative platforms, whereas iOS refers to only iPhone/iPad (Confusing but the case for historical reasons as [documented here](https://chaosinmotion.com/2021/08/02/things-to-remember-compiler-conditionals-for-macos-ios-etc/))
* TBB requires SDKROOT to be passed in or it can often go off and find some random compiler toolchain and fail.
* Add APPLE_EMBEDDED boolean to designate when using the CMake supported cross compilation targets. Added in Options.cmake so it can be used to configure defaults properly.

This PR requires PixarAnimationStudios#2947 to be merged first as that allows for not requiring Boost patches per platform, which can get fragile.
@spiffmon
Copy link
Copy Markdown
Member

Thanks for submitting this, @dgovil , though after consideration, we don't think it's right to take this, just yet, since:

  1. We're concerned about testing against all the different boost versions
  2. Also concerned about hardcoding transitive dependencies, which may change, version to version
  3. We plan to have all boost dependencies except boost::python gone within the next release or two, and after that this PR could be much simpler.

@dgovil
Copy link
Copy Markdown
Collaborator Author

dgovil commented Feb 28, 2024

@spiffmon That's understandable, though most of the header reduction complexity was by request.

If I revert to my initial version of this PR, it just does a straight copy of headers and so is much more straightforward/consistent for all boost versions.

A few alternatives that I'd like to run by you, since this is the basis of our iOS patch and so makes landing those somewhat more complex.

  1. I could revert this to just copy all headers and not care about the excess size. (This matches what b2 does already and turns it into a very small PR)

  2. I could drop this PR and add B2 patches to the iOS PR. That does make it a little more fragile IMHO, but I could limit the boost version for iOS to 1.82.

  3. Leave the iOS patches till after boost is removed.

My worry is that we tie the iOS patches to removal of boost as (IMHO) they could be separable concerns.

Since the PRs are stacked, it's easier to solve them from the root upwards.

@sunyab
Copy link
Copy Markdown
Contributor

sunyab commented Feb 28, 2024

I'd vote to just copy all the headers, and ideally just for header-only libraries. That retains the "don't have to actually build boost" benefits without the maintenance burden, and is no worse from a size perspective than we were before.

@dgovil
Copy link
Copy Markdown
Collaborator Author

dgovil commented Feb 29, 2024

Ooh, I managed to get something working because of the following changes that landed for 24.3

  1. Boost 1.82 fixes some stuff and we'd enabled that for Python 3.11
  2. My other boost patches to work with our compiler toolchains

It means B2 build now succeeds for iPhone derived platforms without any patches to B2's configs, and just minor changes to the b2 args.

So I'll do two things:

  1. I'll uncouple the iOS PR from this completely with that change in place. It just results in some wasted compute and some libs that won't get used.
  2. I'll revert this PR to have no filtering.

That way, we can evaluate this PR on its own merits instead of it being tied to the iOS ones. In which case, maybe let's just close this PR out as unnecessary, in favour of the de-boosting effort? Does that sound good to you, @sunyab and @spiffmon ?

When building USD without Python, OIIO or VDB, Boost libs aren't required.
In this scenario, it is preferable to just copy over the headers instead and save some time and complexity.

This also has a significant benefit that it allows the core USD build to be supported on platforms that Boost doesn't natively build on yet. It means that the build_usd.py script doesn't need to carry as many patches to Boost itself if someone is just building the core USD library.

This is similar to [PR 2914](PixarAnimationStudios#2914) but can short circuit the entire bootstrap and b2 process, which may still error on platforms that b2 doesn't understand.
@dgovil dgovil force-pushed the boost_headers_only branch from b71bf91 to 22baa80 Compare February 29, 2024 00:43
@dgovil
Copy link
Copy Markdown
Collaborator Author

dgovil commented Feb 29, 2024

Okay:

  1. Decoupled all the iOS PRs from this.
  2. Changed this so it does a direct copy of the headers.

But totally feel free to close/reject this PR as well. The only advantage is the time/space savings of not having to run B2 now

@jesschimein
Copy link
Copy Markdown
Collaborator

/AzurePipelines run

@azure-pipelines
Copy link
Copy Markdown

Azure Pipelines successfully started running 1 pipeline(s).

@sunyab
Copy link
Copy Markdown
Contributor

sunyab commented Feb 29, 2024

Great! Since you're no longer depending on this I'm going to close this out since we're not benefiting much from it, and we're also this close to being rid of the boost dependency (in most cases) anyway. Thanks!

@sunyab sunyab closed this Feb 29, 2024
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.

5 participants