Skip to content

Conversation

@GaetanLepage
Copy link
Contributor

@GaetanLepage GaetanLepage commented Sep 8, 2024

Description of changes

Changelogs:

Diff: erincatto/box2d@v2.4.2...v3.1.0

cc @raskin

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 24.11 Release Notes (or backporting 23.11 and 24.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.

Add a 👍 reaction to pull requests you find important.

@GaetanLepage
Copy link
Contributor Author

Currently fails with:

[ 98%] Linking CXX executable ../bin/test
cd /build/source/build/test && /nix/store/hx89vlv61l4vm0dvn0zww817nckzh7ls-cmake-3.29.6/bin/cmake -E cmake_link_script CMakeFiles/test.dir/link.txt --verbose=1
/nix/store/gxpdysvlj9l6bfin03k6yq0s922pk7pg-gcc-wrapper-13.3.0/bin/g++ -O3 -DNDEBUG CMakeFiles/test.dir/main.c.o CMakeFiles/test.dir/test_bitset.c.o CMakeFiles/test.dir/test_collisi>
/build/source/samples/settings.cpp: In function 'bool ReadFile(char*&, int&, const char*)':
/build/source/samples/settings.cpp:35:14: error: ignoring return value of 'size_t fread(void*, size_t, size_t, FILE*)' declared with attribute 'warn_unused_result' [8;;https://gcc.g>
   35 |         fread( data, size, 1, file );
      |         ~~~~~^~~~~~~~~~~~~~~~~~~~~~~
/build/source/samples/shader.cpp: In function 'GLuint sCreateShaderFromFile(const char*, GLenum)':
/build/source/samples/shader.cpp:153:14: error: ignoring return value of 'size_t fread(void*, size_t, size_t, FILE*)' declared with attribute 'warn_unused_result' [8;;https://gcc.gn>
  153 |         fread( source, size, 1, file );
      |         ~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~
cc1plus: all warnings being treated as errors
make[2]: *** [samples/CMakeFiles/samples.dir/build.make:345: samples/CMakeFiles/samples.dir/shader.cpp.o] Error 1
make[2]: *** Waiting for unfinished jobs....
make[2]: Leaving directory '/build/source/build'
cc1plus: all warnings being treated as errors
[ 98%] Built target test
make[2]: *** [samples/CMakeFiles/samples.dir/build.make:331: samples/CMakeFiles/samples.dir/settings.cpp.o] Error 1
make[2]: Leaving directory '/build/source/build'
make[1]: *** [CMakeFiles/Makefile2:338: samples/CMakeFiles/samples.dir/all] Error 2
make[1]: Leaving directory '/build/source/build'
make: *** [Makefile:139: all] Error 2

@GaetanLepage GaetanLepage force-pushed the box2d branch 3 times, most recently from 5d404de to 8fa293a Compare September 8, 2024 09:27
@GaetanLepage
Copy link
Contributor Author

Not sure that my glfw injection is working:

-- Adding Box2D unit tests
CMake Warning at samples/CMakeLists.txt:20 (find_package):
  By not providing "Findglfw.cmake" in CMAKE_MODULE_PATH this project has
  asked CMake to find a package configuration file provided by "glfw", but
  CMake did not find one.

  Could not find a package configuration file provided by "glfw" with any of
  the following names:

    glfwConfig.cmake
    glfw-config.cmake

  Add the installation prefix of "glfw" to CMAKE_PREFIX_PATH or set
  "glfw_DIR" to a directory containing one of the above files.  If "glfw"
  provides a separate development package or SDK, be sure it has been
  installed.


-- Configuring done (1.0s)
-- Generating done (0.0s)

@tobim
Copy link
Contributor

tobim commented Sep 14, 2024

Not sure that my glfw injection is working:

Did you try glfw3?

@GaetanLepage
Copy link
Contributor Author

Did you try glfw3?

It works better thanks, but still fails with:

/build/source/samples/settings.cpp: In function 'bool ReadFile(char*&, int&, const char*)':
/build/source/samples/settings.cpp:35:14: error: ignoring return value of 'size_t fread(void*, size_t, size_t, FILE*)' declared with attribute 'warn_unused_result' [-Werror=unused-result]
   35 |         fread( data, size, 1, file );
      |         ~~~~~^~~~~~~~~~~~~~~~~~~~~~~
/build/source/samples/shader.cpp: In function 'GLuint sCreateShaderFromFile(const char*, GLenum)':
/build/source/samples/shader.cpp:153:14: error: ignoring return value of 'size_t fread(void*, size_t, size_t, FILE*)' declared with attribute 'warn_unused_result' [-Werror=unused-result]
  153 |         fread( source, size, 1, file );
      |         ~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~
cc1plus: all warnings being treated as errors
make[2]: *** [samples/CMakeFiles/samples.dir/build.make:345: samples/CMakeFiles/samples.dir/shader.cpp.o] Error 1
make[2]: *** Waiting for unfinished jobs....
cc1plus: all warnings being treated as errors
make[2]: *** [samples/CMakeFiles/samples.dir/build.make:331: samples/CMakeFiles/samples.dir/settings.cpp.o] Error 1
make[2]: Leaving directory '/build/source/build'
make[1]: *** [CMakeFiles/Makefile2:338: samples/CMakeFiles/samples.dir/all] Error 2
make[1]: Leaving directory '/build/source/build'
make: *** [Makefile:139: all] Error 2

@tobim
Copy link
Contributor

tobim commented Sep 15, 2024

erincatto/box2d#784 might fix that.

@GaetanLepage
Copy link
Contributor Author

GaetanLepage commented Sep 15, 2024

erincatto/box2d#784 might fix that.

Thanks ! I was able to cherry-pick the necessary fixes.
Now it builds fine, however, the result doesn't include the headers anymore:

previous version

[nix-shell:~/nixpkgs]$ tree /nix/store/jlnr0l28rq29y8xrhrn7sdswxycv8kav-box2d-2.4.2
/nix/store/jlnr0l28rq29y8xrhrn7sdswxycv8kav-box2d-2.4.2
├── include
│   └── box2d
│       ├── b2_api.h
│       ├── b2_block_allocator.h
│       ├── b2_body.h
│       ├── b2_broad_phase.h
│       ├── b2_chain_shape.h
│       ├── b2_circle_shape.h
│       ├── b2_collision.h
│       ├── b2_common.h
│       ├── b2_contact.h
│       ├── b2_contact_manager.h
│       ├── b2_distance.h
│       ├── b2_distance_joint.h
│       ├── b2_draw.h
│       ├── b2_dynamic_tree.h
│       ├── b2_edge_shape.h
│       ├── b2_fixture.h
│       ├── b2_friction_joint.h
│       ├── b2_gear_joint.h
│       ├── b2_growable_stack.h
│       ├── b2_joint.h
│       ├── b2_math.h
│       ├── b2_motor_joint.h
│       ├── b2_mouse_joint.h
│       ├── b2_polygon_shape.h
│       ├── b2_prismatic_joint.h
│       ├── b2_pulley_joint.h
│       ├── b2_revolute_joint.h
│       ├── b2_rope.h
│       ├── b2_settings.h
│       ├── b2_shape.h
│       ├── b2_stack_allocator.h
│       ├── b2_time_of_impact.h
│       ├── b2_timer.h
│       ├── b2_time_step.h
│       ├── b2_types.h
│       ├── b2_weld_joint.h
│       ├── b2_wheel_joint.h
│       ├── b2_world_callbacks.h
│       ├── b2_world.h
│       └── box2d.h
└── lib
    ├── cmake
    │   └── box2d
    │       ├── box2dConfig.cmake
    │       ├── box2dConfig-release.cmake
    │       └── box2dConfigVersion.cmake
    └── libbox2d.a

6 directories, 44 files

new version:

[nix-shell:~/nixpkgs]$ tree /nix/store/if07h00dhj0xw8dkcfc0kxr1y6z2bf28-box2d-3.0.0
/nix/store/if07h00dhj0xw8dkcfc0kxr1y6z2bf28-box2d-3.0.0
└── lib
    └── libbox2d.a

2 directories, 1 file

@ofborg ofborg bot requested review from 7c6f434c and guibou September 15, 2024 11:07
@ofborg ofborg bot added 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-linux: 11-100 This PR causes between 11 and 100 packages to rebuild on Linux. labels Sep 15, 2024
@wegank wegank added the 2.status: merge conflict This PR has merge conflicts with the target branch label Sep 27, 2024
@atomicptr
Copy link

Considering box2d 3.0 is not compatible with the 2.x version (which has been renamed to box2d-lite https://github.com/erincatto/box2d-lite), it would probably be nice to have box2d-lite also available when this gets merged to make migrations easier on the projects that depend on the current 2.x version.

@GaetanLepage GaetanLepage force-pushed the box2d branch 2 times, most recently from 3491b56 to fade7e3 Compare October 6, 2024 10:03
@ofborg ofborg bot added 8.has: package (new) This PR adds a new package and removed 2.status: merge conflict This PR has merge conflicts with the target branch labels Oct 6, 2024
@wegank wegank added the 2.status: merge conflict This PR has merge conflicts with the target branch label Nov 9, 2024
@GaetanLepage GaetanLepage force-pushed the box2d branch 3 times, most recently from 18b6671 to 0240249 Compare December 18, 2024 13:25
@ofborg ofborg bot removed the 2.status: merge conflict This PR has merge conflicts with the target branch label Dec 18, 2024
@SomeoneSerge
Copy link
Contributor

-Werror=unused-result

For future reference, you could have also passed -Wno-error=unused-result to the cc-wrapper or CMAKE_CXX_FLAGS

directory structure

Seems like they only recovered the install statement after the release: https://github.com/erincatto/box2d/pull/783/files#diff-1e7de1ae2d059d21e1dd75d5812d5a34b0222cef273b7c3a2af62eb747f9d20aL69-R75

@GaetanLepage
Copy link
Contributor Author

I discussed with the maintainer on Discord about the cmake failures.
Most issues have already been addressed upstream and they will be released as 3.1.0 in ~Feb 2025. I propose to wait for this.

@wegank wegank added the 2.status: merge conflict This PR has merge conflicts with the target branch label Apr 2, 2025
@GaetanLepage GaetanLepage marked this pull request as ready for review April 20, 2025 08:45
@ofborg ofborg bot removed the 2.status: merge conflict This PR has merge conflicts with the target branch label Apr 20, 2025
@github-actions github-actions bot added the 10.rebuild-darwin: 1 This PR causes 1 package to rebuild on Darwin. label Apr 20, 2025
@GaetanLepage GaetanLepage force-pushed the box2d branch 3 times, most recently from 26c8710 to 687f9f4 Compare April 20, 2025 09:34
@GaetanLepage GaetanLepage changed the title box2d: 2.4.2 -> 3.0.0 box2d: 2.4.2 -> 3.1.0 Apr 20, 2025
@GaetanLepage GaetanLepage force-pushed the box2d branch 2 times, most recently from 1f283c0 to 6d93d53 Compare April 20, 2025 10:03
@7c6f434c
Copy link
Member

  1. You never answered about keeping 2.x/lite as a parallel version.
  2. And if you are talking to upstream (unlike me), you should probably add yourself as a maintainer.

@GaetanLepage
Copy link
Contributor Author

You never answered about keeping 2.x/lite as a parallel version.

Sorry, there has been a long pause in this PR. I have updated it now that 3.1.0 is out and that the CMake issues have been fixed. I have prepared the packaging of box2d-light on a separate branch. It builds fine but the CMake contains no rules for installation, so I need to work on it some more.

2. And if you are talking to upstream (unlike me), you should probably add yourself as a maintainer.

My only interaction with upstream happened on Discord a few months ago when I was trying to build 3.0.0. I was facing the aforementioned CMake issues and they told me to wait for 3.1.0.

I can add myself as a maintainer if you wish.

@GaetanLepage
Copy link
Contributor Author

Libreoffice fails to build with this new box2d version.

Do you have any information about this @7c6f434c?

@github-actions github-actions bot removed the 10.rebuild-darwin: 1 This PR causes 1 package to rebuild on Darwin. label Apr 20, 2025
@GaetanLepage
Copy link
Contributor Author

I have ended up opening a PR adding box2d-lite: #400336.
However, it seems unmaintained (last commit is from 6 years ago) and it does not correspond to the latest 2.x box2d release anyway.
Indeed, box2d v2.4.2 has been released on Aug. 2024 and named "Final release of version 2".

In the end, I have added box2d_2 (targeting v2.4.2) in this PR.
It was actually necessary to get LibreOffice building (which is incompatible with Box2d >=3.0).

@GaetanLepage GaetanLepage mentioned this pull request Apr 20, 2025
13 tasks
Copy link
Contributor

Choose a reason for hiding this comment

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

My personal opinion is that whenever we can we should be maintaining just one Nix expression callPackage-d or overrideAttrs-ed with different arguments, because this way we implicitly test their stability wrto overrides. Ofc there's no consensus around that and this comment is not blocking

Copy link
Member

Choose a reason for hiding this comment

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

I think if v2 expression is simpler and unlikely to change much, it makes all the sense to keep it separate and frozen; overrides makes sense when we expect more common changes to both versions than divergent changes.

@7c6f434c
Copy link
Member

Outside the updater question, looks good to go

@GaetanLepage
Copy link
Contributor Author

nixpkgs-review result

Generated using nixpkgs-review.

Command: nixpkgs-review pr 340470


x86_64-linux

✅ 32 packages built:
  • box2d
  • box2d_2
  • collabora-online
  • gcompris
  • gotenberg
  • kdePackages.qmlbox2d (qt6Packages.qmlbox2d)
  • libreoffice (libreoffice-still)
  • libreoffice-collabora
  • libreoffice-fresh
  • libreoffice-fresh-unwrapped
  • libreoffice-qt (libreoffice-qt-still)
  • libreoffice-qt-fresh
  • libreoffice-qt-fresh-unwrapped
  • libreoffice-qt-unwrapped (libreoffice-qt-still-unwrapped, libreoffice-qt6-still-unwrapped)
  • libreoffice-qt6 (libreoffice-qt6-still)
  • libreoffice-qt6-fresh
  • libreoffice-qt6-fresh-unwrapped
  • libreoffice-qt6-unwrapped
  • libreoffice-unwrapped (libreoffice-still-unwrapped)
  • libsForQt5.qmlbox2d (plasma5Packages.qmlbox2d)
  • lomiri.lomiri-docviewer-app
  • paperwork
  • paperwork.dist
  • python312Packages.paperwork-backend
  • python312Packages.paperwork-backend.dist
  • python312Packages.paperwork-shell
  • python312Packages.paperwork-shell.dist
  • python313Packages.paperwork-backend
  • python313Packages.paperwork-backend.dist
  • python313Packages.paperwork-shell
  • python313Packages.paperwork-shell.dist
  • unoconv

aarch64-linux

✅ 32 packages built:
  • box2d
  • box2d_2
  • collabora-online
  • gcompris
  • gotenberg
  • kdePackages.qmlbox2d (qt6Packages.qmlbox2d)
  • libreoffice (libreoffice-still)
  • libreoffice-collabora
  • libreoffice-fresh
  • libreoffice-fresh-unwrapped
  • libreoffice-qt (libreoffice-qt-still)
  • libreoffice-qt-fresh
  • libreoffice-qt-fresh-unwrapped
  • libreoffice-qt-unwrapped (libreoffice-qt-still-unwrapped, libreoffice-qt6-still-unwrapped)
  • libreoffice-qt6 (libreoffice-qt6-still)
  • libreoffice-qt6-fresh
  • libreoffice-qt6-fresh-unwrapped
  • libreoffice-qt6-unwrapped
  • libreoffice-unwrapped (libreoffice-still-unwrapped)
  • libsForQt5.qmlbox2d (plasma5Packages.qmlbox2d)
  • lomiri.lomiri-docviewer-app
  • paperwork
  • paperwork.dist
  • python312Packages.paperwork-backend
  • python312Packages.paperwork-backend.dist
  • python312Packages.paperwork-shell
  • python312Packages.paperwork-shell.dist
  • python313Packages.paperwork-backend
  • python313Packages.paperwork-backend.dist
  • python313Packages.paperwork-shell
  • python313Packages.paperwork-shell.dist
  • unoconv

x86_64-darwin

✅ 2 packages built:
  • box2d
  • box2d_2

aarch64-darwin

✅ 2 packages built:
  • box2d
  • box2d_2

@GaetanLepage
Copy link
Contributor Author

Outside the updater question, looks good to go

I removed the updater from box2d_2, so I will now merge the PR. Thanks all for the review!

@GaetanLepage GaetanLepage merged commit a602d8a into NixOS:master Apr 20, 2025
23 of 27 checks passed
@GaetanLepage GaetanLepage deleted the box2d branch April 20, 2025 18:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

8.has: package (new) This PR adds a new package 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-linux: 11-100 This PR causes between 11 and 100 packages to rebuild on Linux.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants