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

discordo: init at unstable-2023-04-07 #210667

Merged
merged 2 commits into from
Apr 8, 2023
Merged

Conversation

Arian-D
Copy link
Contributor

@Arian-D Arian-D commented Jan 14, 2023

Description of changes
Things done
  • 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, 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/)
  • 23.05 Release Notes (or backporting 22.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
    • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
  • Fits CONTRIBUTING.md.

Copy link
Member

@lilyinstarlight lilyinstarlight left a comment

Choose a reason for hiding this comment

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

This seems to build just fine for me on x86_64-linux (though I did not actually try to run it) and the derivation looks good

@SuperSandro2000 SuperSandro2000 added the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Jan 23, 2023
@Arian-D Arian-D changed the title discordo: init at unstable-2023-01-09 discordo: init at unstable-2023-02-02 Feb 2, 2023
@Arian-D
Copy link
Contributor Author

Arian-D commented Feb 2, 2023

Just a mini update on this:

  • I tested it on aarch64 (non-nixos)
  • Updated the commit to a more recent version.

@mweinelt mweinelt removed the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Feb 5, 2023
@spikespaz
Copy link
Contributor

What about clipboard support?

Should add them to buildInputs.

@lilyinstarlight
Copy link
Member

What about clipboard support?

Should add them to buildInputs.

Adding them to buildInputs won't do anything because they are needed at runtime. We could wrapProgram the main binary in the derivation to always have those in PATH, though

Something like:

buildGoModue rec {
  #stuff...
  nativeBuildInputs = [ makeWrapper ];
  #stuff...
  postInstall = ''
    wrapProgram $out/bin/discordo \
      --prefix PATH : ${lib.makeBinPath [ xsel wl-clipboard ]}
  '';
}

Copy link
Contributor

@adamcstephens adamcstephens left a comment

Choose a reason for hiding this comment

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

Looks good on darwin, too. This commit is now a month behind upstream.

Result of nixpkgs-review pr 210667 run on aarch64-darwin 1

1 package built:
  • discordo

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

1 package built:
  • discordo

@figsoda figsoda added the 12.approvals: 2 This PR was reviewed and approved by two reputable people label Mar 5, 2023
@lilyinstarlight
Copy link
Member

Do you think you could bump this to a newer commit? I think it's probably merge-ready

Also if you want to learn something cool for automating the updates, you can add unstableGitUpdater to the file args and add passthru.updateScript = unstableGitUpdater { }; to the derivation, and then r-ryantm should occasionally pick up updates automatically and you can run the update script manually like nix-shell maintainers/scripts/update.nix --argstr commit true --argstr package discordo

@Arian-D
Copy link
Contributor Author

Arian-D commented Mar 28, 2023

I apologize about the delay; for some reason I was not getting notification until the most recent message.

  • Per spikespaz's requeset I added xsel and wl-clipboard to the definition
  • I added the ldflags per Kranzes' suggestion
  • I bumped it up to a more recent version and added the unstable updater. Thank you lilyinstarlight for the guidance and advice

@adamcstephens
Copy link
Contributor

You'll want to update the title of the PR to match the new commit description.

This also builds and runs successfully as a static binary, so I'd suggest adding CGO_ENABLED = 0;

@lilyinstarlight
Copy link
Member

  • I bumped it up to a more recent version and added the unstable updater. Thank you lilyinstarlight for the guidance and advice

So I think I didn't notice it was a Go module when I said that, you actually need to use nix-update (instead of unstableGitUpdater) so that the vendorHash gets updated too:

  passthru.updateScript = nix-update-script {
    extraArgs = [ "--version=branch" ];
  };

@Arian-D Arian-D changed the title discordo: init at unstable-2023-02-02 discordo: init at unstable-2023-04-07 Apr 7, 2023
@SuperSandro2000 SuperSandro2000 merged commit 0885905 into NixOS:master Apr 8, 2023
@Janik-Haag Janik-Haag added the 12. first-time contribution This PR is the author's first one; please be gentle! label Jun 12, 2023
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 10.rebuild-darwin: 1 10.rebuild-linux: 1-10 10.rebuild-linux: 1 11.by: package-maintainer This PR was created by the maintainer of the package it changes 12.approvals: 2 This PR was reviewed and approved by two reputable people 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.

9 participants