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

sfml: 2.5.1 -> 2.6.1; csfml: 2.5.2 -> 2.6.1 #312161

Open
wants to merge 17 commits into
base: master
Choose a base branch
from
Open

sfml: 2.5.1 -> 2.6.1; csfml: 2.5.2 -> 2.6.1 #312161

wants to merge 17 commits into from

Conversation

drawbu
Copy link
Contributor

@drawbu drawbu commented May 16, 2024 •
edited
Loading

Description of changes

  • Just updated the packages
  • Moved to by name
  • Reformat with nixfmt-rfc-style

Works fine on a few projects of mine on x86_64-linux
Managed to get it working on x86_64-darwin

CSFML & SFML packages don't get a lot of updates on the nixpkgs. I might add myself as a maintainer if we don't get a reply shortly

I also had to patch pkgconfig path in the cmake as it is broken upstream. A or fixes the issue for SFML package and I replicated the patch for CSFML.

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.05 Release Notes (or backporting 23.05 and 23.11 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.

@drawbu

This comment was marked as outdated.

Sign in to view
@ofborg ofborg bot requested review from jpdoyle and astsmtl May 16, 2024 09:29
@drawbu
Copy link
Contributor Author

drawbu commented May 16, 2024

@ofborg build sfml csfml

@drawbu drawbu marked this pull request as ready for review May 17, 2024 15:41
@drawbu drawbu self-assigned this May 17, 2024
@wegank wegank added the 12.approvals: 1 This PR was reviewed and approved by one reputable person label May 17, 2024
@drawbu
Copy link
Contributor Author

drawbu commented May 18, 2024

Result of nixpkgs-review pr 312161 run on x86_64-darwin 1

3 packages marked as broken and skipped:
  • dolphin-emu-primehack
  • libretro.dolphin
  • retroarchFull
9 packages built:
  • antsimulator
  • cutecapture
  • dolphin-emu
  • openrw
  • python311Packages.nocturne
  • python311Packages.nocturne.dist
  • python312Packages.nocturne
  • python312Packages.nocturne.dist
  • sfml

@drawbu
Copy link
Contributor Author

drawbu commented May 20, 2024

Result of nixpkgs-review pr 312161 run on x86_64-linux 1

1 package marked as broken and skipped:
  • shadered
25 packages built:
  • EmptyEpsilon
  • antsimulator
  • attract-mode
  • csfml
  • cutecapture
  • dolphin-emu
  • dolphin-emu-primehack
  • extremetuxracer
  • guvcview
  • libretro.dolphin
  • lutris
  • lutris-free
  • marble-marcher-ce
  • mars
  • opendungeons
  • openrw
  • python311Packages.nocturne
  • python311Packages.nocturne.dist
  • python312Packages.nocturne
  • python312Packages.nocturne.dist
  • qstopmotion
  • retroarchFull
  • sfml
  • simplenes
  • vbam

@wegank wegank removed the 12.approvals: 1 This PR was reviewed and approved by one reputable person label May 20, 2024
@drawbu
Copy link
Contributor Author

drawbu commented May 21, 2024

pkg-config is broken upsream.
See SFML/SFML#2835

@Sigmanificient
Copy link
Member

Sigmanificient commented May 21, 2024 •
edited
Loading

When building the csfml package without the patches, nix gives the following error:

Broken paths found in a .pc file! /nix/store/g230r8pdkgzfwjn7a2c9r7vapidg2dkg-csfml-2.6.1/lib/pkgconfig/csfml-network.pc
The following lines have issues (specifically '//' in paths).
3:libdir=${exec_prefix}//nix/store/g230r8pdkgzfwjn7a2c9r7vapidg2dkg-csfml-2.6.1/lib
It is very likely that paths are being joined improperly.
ex: "${prefix}/@CMAKE_INSTALL_LIBDIR@" should be "@CMAKE_INSTALL_FULL_LIBDIR@"
Please see https://github.com/NixOS/nixpkgs/issues/144170 for more details.

Following nix advise, I tried to build with these changes:

--- a/pkgs/by-name/cs/csfml/package.nix    2024-05-21 09:41:18.068576345 +0200
+++ b/pkgs/by-name/cs/csfml/package.nix    2024-05-21 09:40:58.865882958 +0200
@@ -20,7 +20,11 @@
   nativeBuildInputs = [ cmake ];
   buildInputs = [ sfml ];
   cmakeFlags = [ "-DCMAKE_MODULE_PATH=${sfml}/share/SFML/cmake/Modules/" ];
-  patches = [ ./pkgconfig.patch ];
+
+  prePatch = ''
+   substituteInPlace tools/pkg-config/* \
+     --replace-fail 'libdir=''${exec_prefix}/@CMAKE_INSTALL_LIBDIR@' "libdir=@CMAKE_INSTALL_FULL_LIBDIR@"
+  '';
 
   meta = {
     homepage = "https://www.sfml-dev.org/";

It produces a valid pkg-config file:

image

@drawbu
Copy link
Contributor Author

drawbu commented May 21, 2024

dev out change broke csfml. Currently trying to fix the issue

@drawbu
Copy link
Contributor Author

drawbu commented May 22, 2024

Removing multiple outputs as it is broken right now for SFML/CSFML

@drawbu
Copy link
Contributor Author

drawbu commented May 22, 2024 •
edited
Loading

@ofborg build sfml csfml

@drawbu
Copy link
Contributor Author

drawbu commented May 22, 2024 •
edited
Loading

When the build ends, the pr should be good to go. Waiting for review by maintainers

@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-ready-for-review/3032/3979

@drawbu
Copy link
Contributor Author

drawbu commented Jun 6, 2024

We still have no answer from the maintainers of both sfml and csfml after 3 weeks. I'm adding myself as maintainer

@ofborg ofborg bot added the 11.by: package-maintainer This PR was created by the maintainer of the package it changes label Jun 6, 2024
SuperSandro2000
SuperSandro2000 reviewed Jun 6, 2024
View reviewed changes
pkgs/by-name/cs/csfml/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/cs/csfml/package.nix
meta = with lib; {
prePatch = ''
substituteInPlace tools/pkg-config/* \
--replace-fail 'libdir=''${exec_prefix}/@CMAKE_INSTALL_LIBDIR@' "libdir=@CMAKE_INSTALL_FULL_LIBDIR@"
Copy link
Member

Choose a reason for hiding this comment

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

Please try to upstream this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry for late response
SFML/CSFML#374

@wegank wegank removed the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Jun 20, 2024
@wegank wegank added the 2.status: merge conflict This PR has merge conflicts with the target branch label Sep 10, 2024
drawbu and others added 16 commits October 13, 2024 11:17
@drawbu
sfml: 2.5.1 -> 2.6.1
be8b504
@drawbu
csfml: 2.5.2 -> 2.6.0
4185a7e
@drawbu
sfml: use correct hash
bd2dca4
@drawbu
sfml: reformat with nixfmt-rfc-style
007724e
@drawbu
csfml: reformat with nixfmt-rfc-style
d92cae9
@drawbu @Sigmanificient
sfml: use finalAttrs instead of rec
0ccfd7f 
Co-authored-by: Sigmanificient <edhyjox@gmail.com>
@drawbu @Sigmanificient
csfml: use finalAttrs instead of rec
a8fc0dd 
Co-authored-by: Sigmanificient <edhyjox@gmail.com>
@drawbu
sfml: move to by-name
8fd5b41
@drawbu
csfml: move to by-name
5a41764 
move to byname
@drawbu
sfml: fix pkg-config
2221cab
@drawbu
sfml: fix pkg-config
2c67068
@drawbu
csfml: 2.6.0 -> 2.6.1
37fb5de
@drawbu
csfml: mark platform to unix instead of linux
0de0712
@drawbu @Sigmanificient
csfml: use substitutePatch instead of file patch
c15a08a 
Co-authored-by: Yohann Boniface <edhyjox@gmail.com>
@drawbu
{csfml,sfml}: add drawbu to meta.maintainers
63ba299
@drawbu @SuperSandro2000
csfml: replace prePatch with postPatch
aac0c39 
Co-authored-by: Sandro <sandro.jaeckel@gmail.com>
@drawbu
Copy link
Contributor Author

drawbu commented Oct 13, 2024

fixed merge conflict with e0464e4

@drawbu
Copy link
Contributor Author

drawbu commented Oct 13, 2024

One this pr is good to go, i will squash all that mess

@drawbu drawbu changed the title sfml: 2.5.1 -> 2.6.1; csfml: 2.5.2 -> 2.6.0 sfml: 2.5.1 -> 2.6.1; csfml: 2.5.2 -> 2.6.1 Oct 13, 2024
@ofborg ofborg bot removed the 2.status: merge conflict This PR has merge conflicts with the target branch label Oct 13, 2024
@drawbu drawbu mentioned this pull request Oct 13, 2024
Open
@drawbu
Copy link
Contributor Author

drawbu commented Oct 13, 2024

When my PR gets merged upstream in the CSFML repo, all patches can be deleted when the 3.0.0 (works for the moment at least)

@wegank wegank added the 2.status: merge conflict This PR has merge conflicts with the target branch label Nov 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SuperSandro2000 SuperSandro2000 SuperSandro2000 left review comments

@Sigmanificient Sigmanificient Sigmanificient approved these changes

@jpdoyle jpdoyle Awaiting requested review from jpdoyle

@astsmtl astsmtl Awaiting requested review from astsmtl

Assignees

@drawbu drawbu

Labels
2.status: merge conflict This PR has merge conflicts with the target branch 8.has: package (new) This PR adds a new package 10.rebuild-darwin: 11-100 10.rebuild-linux: 11-100 11.by: package-maintainer This PR was created by the maintainer of the package it changes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@drawbu @Sigmanificient @nixos-discourse @SuperSandro2000 @wegank