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

nextcloud-client: fix dbus service location #368873

Merged
merged 1 commit into from
Dec 28, 2024

Conversation

kleifgch
Copy link
Contributor

I see no reason, why this service file would be placed in a location where dbus does not expect it.

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/)
  • 25.05 Release Notes (or backporting 24.11 and 25.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.

@kleifgch
Copy link
Contributor Author

@Ma27 you seem to have written the original patch.
Maybe you can tell me if I missed something.

@github-actions github-actions bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 labels Dec 28, 2024
@NixOSInfra NixOSInfra added the 12. first-time contribution This PR is the author's first one; please be gentle! label Dec 28, 2024
@lucasew
Copy link
Contributor

lucasew commented Dec 28, 2024

Isn't this a no-op? Isn't the build system using the value provided by pkg-config? If this solves the issue, did you try to upstream the patch?

@lucasew
Copy link
Contributor

lucasew commented Dec 28, 2024

I answered my two questions by simply building the package on master.

The issue is there.

@lucasew
Copy link
Contributor

lucasew commented Dec 28, 2024

Upstream changed a little this logic. Probably it will be solved in the next bump.

I didn't know one could pass a variable to override a value in pkg-config tho

https://github.com/nextcloud/desktop/blob/ef1d2a34cc0bbc6e5c7631648c89c6665c9e6fc0/shell_integration/libcloudproviders/CMakeLists.txt#L3

@lucasew
Copy link
Contributor

lucasew commented Dec 28, 2024

Latest version rn is 3.15.2 tho

@kleifgch
Copy link
Contributor Author

Thanks for your comments!
I did not previously think about that change in upstream, but you're right that it is relevant.
That change is 8 months old and has landed well before the 3.14.3 that is currently in nixpkgs.
That has not fixed the problem yet, because the applied path explicitly overrides that again.
I just tried removing the patch entirely and the service indeed gets installed correctly now,
so this is probably the best solution.

The removed patch was intended to install the dbus service in the proper
location, but due to a typo (`service` vs. `services`) never did. In the
meantime, upstream introduced commit [1] making the patch unnecessary.

To check that everything is in order, one can build nextcloud-client and
verify that the service file is located at $out/share/dbus-1/services/.

[1] nextcloud/desktop@6e1e8a8
@kleifgch kleifgch force-pushed the fix-nextcloud-client-dbus branch from 0863ff7 to 41d9661 Compare December 28, 2024 20:25
@kleifgch
Copy link
Contributor Author

@lucasew Would you mind taking another look?

@lucasew
Copy link
Contributor

lucasew commented Dec 28, 2024

I'm cherry-picking this to my branch to test

I'm updating the package, I hope it doesn't conflict. If the stuff is in services instead of service then LGTM.

#368914

@lucasew
Copy link
Contributor

lucasew commented Dec 28, 2024

For context:

  • This package is emiting the dbus services into $out/share/dbus-1/service
  • It should emit into $out/share/dbus-1/services as you can check in /run/current-system/sw

@lucasew
Copy link
Contributor

lucasew commented Dec 28, 2024

image

Nice

Copy link
Contributor

@GaetanLepage GaetanLepage left a comment

Choose a reason for hiding this comment

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

Diff LGTM ^^

@GaetanLepage
Copy link
Contributor

nixpkgs-review result

Generated using nixpkgs-review.

Command: nixpkgs-review pr 368873


x86_64-linux

✅ 2 packages built:
  • nextcloud-client
  • nextcloud-client.dev

aarch64-linux

✅ 2 packages built:
  • nextcloud-client
  • nextcloud-client.dev

@GaetanLepage GaetanLepage merged commit 6542ebe into NixOS:master Dec 28, 2024
24 of 25 checks passed
@kleifgch kleifgch deleted the fix-nextcloud-client-dbus branch December 28, 2024 22:44
khaneliman pushed a commit to khaneliman/nixpkgs that referenced this pull request Dec 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 10.rebuild-linux: 1 12. first-time contribution This PR is the author's first one; please be gentle!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants