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.

Sign up for GitHub

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

Jump to bottom

Improve Bazel Build #1060

Merged
merged 2 commits into from
Jun 24, 2021
Merged

Improve Bazel Build #1060

merged 2 commits into from
Jun 24, 2021

Conversation

Vertexwahn
Copy link
Contributor

@Vertexwahn Vertexwahn commented Jun 22, 2021 •
edited
Loading

This PR improves the Bazel build (does not affect the CMake build):

  1. It switches from using lpthread to pthread.
    There are good reasons to use pthread instead o lpthread. Details here.
  2. If build as DLL on Windows: windows_export_all_symbols is enabled

@Vertexwahn Vertexwahn force-pushed the bazel-support-switch-to-pthread branch from 96ce4c7 to c54d37b Compare June 22, 2021 21:47
@Vertexwahn Vertexwahn changed the title Swtich to pthread Bazel build: Swtich to pthread Jun 22, 2021
@Vertexwahn Vertexwahn force-pushed the bazel-support-switch-to-pthread branch from 1a372a3 to b32e8b6 Compare June 22, 2021 22:20
@Vertexwahn Vertexwahn changed the title Bazel build: Swtich to pthread Improve Bazel Build + Sort CMake source files alphabetically Jun 22, 2021
@Vertexwahn Vertexwahn force-pushed the bazel-support-switch-to-pthread branch 2 times, most recently from 13a6803 to 3f866a7 Compare June 22, 2021 22:32
meshula
meshula reviewed Jun 23, 2021
View reviewed changes
src/lib/IlmThread/CMakeLists.txt Outdated
OpenEXR::Iex
OpenEXR::Config
Copy link
Contributor

Choose a reason for hiding this comment

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

Not alphabetical ;)

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 will open up an own PR for the CMake "refactoring"

meshula
meshula reviewed Jun 23, 2021
View reviewed changes
Copy link
Contributor

@meshula meshula left a comment

Choose a reason for hiding this comment

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

This looks good to me, but would you mind breaking it apart into two PRs? We prefer to have a single issue addressed in a PR, rather than solving multiple unrelated issues in the same PR.

@Vertexwahn Vertexwahn force-pushed the bazel-support-switch-to-pthread branch from 4a4bc60 to 07711d0 Compare June 24, 2021 15:48
@cary-ilm
Copy link
Member

As @meshula said, this all looks fine but would be better as separate PR's, one for the Bazel improvements and one for sorting the CMakeLists.txt. Can you split them out that way? Thanks.

@Vertexwahn
Swtich to pthread
428958a 
Signed-off-by: Vertexwahn <julian.amann@tum.de>
@Vertexwahn
If build as DLL on Windows enable windows_export_all_symbols
92386d1 
Signed-off-by: Vertexwahn <julian.amann@tum.de>
@Vertexwahn Vertexwahn force-pushed the bazel-support-switch-to-pthread branch from 07711d0 to 92386d1 Compare June 24, 2021 16:59
@Vertexwahn Vertexwahn changed the title Improve Bazel Build + Sort CMake source files alphabetically Improve Bazel Build Jun 24, 2021
@Vertexwahn
Copy link
Contributor Author

Vertexwahn commented Jun 24, 2021 •
edited
Loading

@meshula @cary-ilm You are right - this was not clean - this PR now only changes only one thing - the Bazel build (CMakeLists.txt change is here)

cary-ilm
cary-ilm approved these changes Jun 24, 2021
View reviewed changes
Copy link
Member

@cary-ilm cary-ilm left a comment

Choose a reason for hiding this comment

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

LGTM

@cary-ilm cary-ilm merged commit e8c3370 into AcademySoftwareFoundation:master Jun 24, 2021
@Vertexwahn Vertexwahn deleted the bazel-support-switch-to-pthread branch June 30, 2021 21:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@meshula meshula meshula left review comments

@cary-ilm cary-ilm cary-ilm approved these changes

Assignees
No one assigned
Labels
v3.2.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Vertexwahn @cary-ilm @meshula