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

glm: 0.9.8.5 -> 0.9.9.8 #134913

Merged
merged 1 commit into from Sep 17, 2021
Merged

glm: 0.9.8.5 -> 0.9.9.8 #134913

merged 1 commit into from Sep 17, 2021

Conversation

smancill
Copy link
Contributor

@smancill smancill commented Aug 20, 2021

Motivation for this change

Bump abandoned package to close #132479.

I don't really use it. I was just skimming through the list of issues to find something I may help with.

Things done
Bump version and refactor derivation to make it work
  • Use fetchFromGitHub to download the archive.

  • No need for the updated platform.h patch. The released archive builds with default stdenv. (It may be needed again in the future?)

  • Use the proper attribute to set CMake flags.

  • Don't build the (dummy?) libraries. They are not used anyway by any other target nor made available by the glmConfig.cmake file.

  • Build and run the tests now.

  • Don't define the GLM_COMPILER macro on Darwin. Doesn't seem necessary anymore, there are no errors without it. (And it is not even documented what was the original error that required it to be defined.)

  • Upstream became package-unfriendly and removed the install command, so it needs to be installed manually.

    • Don't set the now unsupported CMake option GLM_INSTALL_ENABLE.

    • Remove unwanted files from the include directory.

    • Fix glmConfig.cmake with the proper path to the include directory.

    • While on it, install a custom pkg-config file, since it is trivial.

    • Don't install unnecessary files as documentation.

  • Since I practically rewrote the entire derivation and the package is abandoned, add myself as maintainer (I guess?).


  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • 21.11 Release Notes (or backporting 21.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

@risicle
Copy link
Contributor

risicle commented Aug 21, 2021

@ofborg build kicad-small

@risicle
Copy link
Contributor

risicle commented Aug 21, 2021

nixpkgs-review reveals no new failures on macos 10.15

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/prs-already-reviewed/2617/404

Bump abandoned package to close NixOS#132479.

- Use fetchFromGitHub to download the archive.

- No need for the updated platform.h patch. The released archive builds
  with default stdenv. (It may be needed again in the future?.)

- Use the proper attribute to set CMake flags.

- Don't build the (dummy?) libraries. They are not used anyway by any
  other target nor made available by the glmConfig.cmake file.

- Build and run the tests now.

- Don't define the GLM_COMPILER macro on Darwin. Doesn't seem necessary
  anymore, there are no errors without it. (And it is not even documented
  what was the original error that required it to be defined.)

- Upstream became package-unfriendly and removed the install command,
  so it needs to be installed manually.

  - Don't set the now unsupported CMake option GLM_INSTALL_ENABLE.

  - Remove unwanted files from the include directory.

  - Fix glmConfig.cmake with the proper path to the include directory.

  - While on it, install a custom pkg-config file, since it is trivial.

  - Don't install unnecessary files as documentation.

- Since I practically rewrote the entire derivation and the package is
  abandoned, add myself as maintainer (I guess?).
@trofi
Copy link
Contributor

trofi commented Sep 21, 2021

At least EmptyEpsilon seems to have regressed since this commit and fails to find -lglm: https://hydra.nixos.org/build/153551301

/nix/store/kmqs0wll31ylwbqkpmlgbjrn6ny3myik-binutils-2.35.1/bin/ld: cannot find -lglm

@smancill
Copy link
Contributor Author

I'll take I look, but I tested the installation with upstream's own cmake test and it is fine:

$ cd ~/src/repos/glm/test/cmake

$ nix-shell -I nixpkgs=$HOME/src/repos/nixpkgs -p glm cmake

[nix-shell:~/src/repos/glm/test/cmake]$ echo $buildInputs
/nix/store/hfd22c9w2m0zh60pzpxava93wb8c0j1g-glm-0.9.9.8 /nix/store/vd7nmzh2pmi0bw35i0gj9m5zh9q1bhc9-cmake-3.21.2

[nix-shell:~/src/repos/glm/test/cmake]$ cat CMakeLists.txt
cmake_minimum_required(VERSION 3.2 FATAL_ERROR)
project(test_find_glm)

find_package(glm REQUIRED)

add_executable(test_find_glm test_find_glm.cpp)
target_link_libraries(test_find_glm glm::glm)

[nix-shell:~/src/repos/glm/test/cmake]$ cmake -S . -B build
-- The C compiler identification is Clang 7.1.0
-- The CXX compiler identification is Clang 7.1.0
-- Detecting C compiler ABI info
-- Detecting C compiler ABI info - done
-- Check for working C compiler: /nix/store/9plx589i75lxgh52ixwjbhhmh5xfv13v-clang-wrapper-7.1.0/bin/clang - skipped
-- Detecting C compile features
-- Detecting C compile features - done
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - done
-- Check for working CXX compiler: /nix/store/9plx589i75lxgh52ixwjbhhmh5xfv13v-clang-wrapper-7.1.0/bin/clang++ - skipped
-- Detecting CXX compile features
-- Detecting CXX compile features - done
-- Configuring done
-- Generating done
-- Build files have been written to: /Users/smancill/src/repos/glm/test/cmake/build

[nix-shell:~/src/repos/glm/test/cmake]$ cmake --build build
[ 50%] Building CXX object CMakeFiles/test_find_glm.dir/test_find_glm.cpp.o
[100%] Linking CXX executable test_find_glm
[100%] Built target test_find_glm

[nix-shell:~/src/repos/glm/test/cmake]$ ./build/test_find_glm
matrix diagonal: 0.489152, 1.05934, -0.237555, 1

@smancill
Copy link
Contributor Author

smancill commented Sep 21, 2021

Mmm, but there is no libglm.so to link with -lglm, it is a header only library.

There is no such library on the previous version neither:

$ tree /nix/store/l920xic0l8rs670ygc6bm6890gy9mwh7-glm-0.9.8.5/lib/
/nix/store/l920xic0l8rs670ygc6bm6890gy9mwh7-glm-0.9.8.5/lib/
├── cmake
│   └── glm
│       ├── glmConfig.cmake
│       ├── glmConfigVersion.cmake
│       └── glmTargets.cmake
└── pkgconfig
    └── glm.pc

@trofi
Copy link
Contributor

trofi commented Sep 21, 2021

I suspect -lglm reference comes from nixpkgs patch: https://github.com/NixOS/nixpkgs/blob/master/pkgs/games/empty-epsilon/0001-bundle-system-glm-in-seriousproton.patch (cc @Ma27 )

But I know almost nothing about cmake's target_link_libraries() mechanics.

@smancill
Copy link
Contributor Author

Well, that patch is wrong.

I'll fix it and send a PR.

@Ma27
Copy link
Member

Ma27 commented Sep 21, 2021

Well, that patch is wrong.

Well, back then it was valid, because it fixed a bunch of CMake-related issues in EmptyEpsilon :-)

It seems as if the lib/cmake has slightly changed which is why CMake erronesouly assumes -lglm now. However, I don't have the specifics in mind currently, so if you're willing to fix it, that'd be appreciated :)

@smancill
Copy link
Contributor Author

smancill commented Sep 21, 2021

The CMake config files did change, since upstream decided to not support proper installation anymore, and it is using now a custom, simplified config file.

But the patch is wrong now because upstream fixed the CMake target name. Before, glm was the target name, now it is glm::glm (which is the right format), and glm is taken as a library file name, which results in -lglm added to the linker flags.

So it is an easy fix, just use the proper target name

@Ma27
Copy link
Member

Ma27 commented Sep 21, 2021

But the patch is wrong now because upstream fixed the CMake target name. Before, glm was the target name, now it is glm::glm (which is the right format), and glm is taken as a file name, which results in -lglm added to the linker flags.

Aah, now I get it, thanks for elaborating!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

glm is out of date (0.9.8.5 -> 0.9.9.8)
6 participants