Skip to content

[glm] Remove all unnecessary build steps#6410

Merged
ras0219-msft merged 3 commits intomicrosoft:masterfrom
cwfitzgerald:glm-fix
May 14, 2019
Merged

[glm] Remove all unnecessary build steps#6410
ras0219-msft merged 3 commits intomicrosoft:masterfrom
cwfitzgerald:glm-fix

Conversation

@cwfitzgerald
Copy link
Contributor

GLM is a header only library. For some reason it builds static and shared libraries within its cmakelists but never actually installs them. In addition, the 160 test executables are unconditionally built with no option to disable them. I have replaced their complicated CMakeLists.txt with a barebones one that properly creates the exported target and copies the headers. Installation time for this header only library went from 3 minutes to 3 seconds.

@NancyLi1013
Copy link
Contributor

Hi @cwfitzgerald, thanks for your new PR. I see that the port globjects that depends on glm failed on all triplets. The port forge failed on x64-windows. Please check the changes again.

x64-windows master test notes
globjects Pass Fail Regression
forge Pass Fail Regression
x64-windows-static master test notes
globjects Pass Fail Regression
x64-osx master test notes
globjects Pass Fail Regression
arm64-windows master test notes
globjects Pass Fail Regression
x86-windows master test notes
globjects Pass Fail Regression
x64-linux master test notes
globjects Pass Fail Regression
x64-uwp master test notes
globjects Pass Fail Regression
arm-uwp master test notes
globjects Pass Fail Regression

failureLogs.zip

@cwfitzgerald
Copy link
Contributor Author

Thanks for the logs, I'll investigate the issue tomorrow.

@cwfitzgerald
Copy link
Contributor Author

cwfitzgerald commented May 13, 2019

globjects failed because of a really stupid header export issue that exported everything inside the glm folder not the folder itself. Not sure if forge is still failing on this build. Perusing the forge log indicates that it should be solved by that fix.

@ras0219-msft
Copy link
Contributor

I confirmed that this shouldn't break CMake users -- the existing glm port provides:

The package glm:x86-windows provides CMake targets:

    find_package(glm CONFIG REQUIRED)
    target_link_libraries(main PRIVATE glm)

@cwfitzgerald
Copy link
Contributor Author

👍 I don't know what is causing the CI to fail currently, but the outward effects of the port should be currently identical.

@ras0219-msft
Copy link
Contributor

This LGTM, thanks for the PR!

@ras0219-msft ras0219-msft merged commit d539182 into microsoft:master May 14, 2019
@ras0219-msft ras0219-msft self-assigned this May 14, 2019
@cwfitzgerald cwfitzgerald deleted the glm-fix branch May 14, 2019 01:20
@cenit
Copy link
Contributor

cenit commented May 15, 2019

I confirmed that this shouldn't break CMake users -- the existing glm port provides:

The package glm:x86-windows provides CMake targets:

    find_package(glm CONFIG REQUIRED)
    target_link_libraries(main PRIVATE glm)

#6461 the new port file does not export interface include directories (which for a header only library are maybe the only important thing)... @ras0219-msft

edit: the CI was also clearly exposing it:

fatal error C1083: Cannot open include file: 'glm/fwd.hpp'

@cwfitzgerald
Copy link
Contributor Author

cwfitzgerald commented May 15, 2019 via email

@ras0219-msft
Copy link
Contributor

The issue was that include(GNUInstallDirs) was placed after the use of CMAKE_INSTALL_INCLUDEDIR, so it wasn't defined yet.

I've opened #6466 to fix this.

@cwfitzgerald
Copy link
Contributor Author

cwfitzgerald commented May 15, 2019 via email

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.

4 participants