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

wsjtz: init at 2.7.0-rc7-1.43 #367971

Merged
merged 2 commits into from
Jan 7, 2025
Merged

wsjtz: init at 2.7.0-rc7-1.43 #367971

merged 2 commits into from
Jan 7, 2025

Conversation

scd31
Copy link

@scd31 scd31 commented Dec 24, 2024 •
edited
Loading

WSJT-Z is a fork of WSJT-X. It's used for ham radio stuff (mainly two-way digital communication). This fork is primarily designed for automation and quality-of-life improvements. Homepage: https://sourceforge.net/projects/wsjt-z/

My first nixpkgs PR :3

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.

@liberodark
Copy link
Contributor

liberodark commented Dec 24, 2024 •
edited
Loading

Hi scd31

You need to set your package in nixpkgs/pkgs/by-name/ws/
Also pkgs/top-level/all-packages.nix is not needed now.
And last point you need to rename your commit wsjtz: init at 2.7.0-rc7-1.43

Best Regards

@NixOSInfra NixOSInfra added the 12. first-time contribution This PR is the author's first one; please be gentle! label Dec 24, 2024
@scd31 scd31 force-pushed the add-wsjtz branch 2 times, most recently from 3f9c756 to 426498a Compare December 24, 2024 17:46
@github-actions github-actions bot added the 8.has: maintainer-list (update) This PR changes `maintainers/maintainer-list.nix` label Dec 24, 2024
@liberodark
Copy link
Contributor

Hi,

Also i think you can change your meta to :

meta = {
    description = "WSJT-X fork, primarily focused on automation and enhanced functionality";
    homepage = "https://sourceforge.net/projects/wsjt-z/";
    license = lib.licenses.gpl3;
    platforms = platforms.linux;
    maintainers = with lib.maintainers; [ scd31 ];
    mainProgram = "wsjtz";
  };

Best Regards

@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 24, 2024
@scd31
Copy link
Author

scd31 commented Dec 24, 2024

@liberodark Thank you for the feedback :3 it has been addressed!

@ofborg ofborg bot added 8.has: package (new) This PR adds a new package 11.by: package-maintainer This PR was created by the maintainer of the package it changes 10.rebuild-linux: 1 labels Dec 25, 2024
drupol
drupol requested changes Dec 29, 2024
View reviewed changes
Copy link
Contributor

@drupol drupol left a comment

Choose a reason for hiding this comment

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

Please make a separate commit for adding you as maintainer.

pkgs/by-name/ws/wsjtz/package.nix Outdated Show resolved Hide resolved
@scd31 scd31 force-pushed the add-wsjtz branch 2 times, most recently from 17966bd to 51d6e98 Compare December 29, 2024 22:40
@scd31
Copy link
Author

scd31 commented Dec 29, 2024

feedback has been addressed :3

@GaetanLepage
Copy link
Contributor

Please, rename the PR and the package commit as wsjts: init at 2.7.0-rc7-1.43

l1npengtul
l1npengtul suggested changes Jan 7, 2025
View reviewed changes
Copy link
Contributor

@l1npengtul l1npengtul left a comment

Choose a reason for hiding this comment

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

Please re-order the commits per https://github.com/NixOS/nixpkgs/blob/master/CONTRIBUTING.md#Commit%20conventions. Specifically, the commit adding you as maintainer should come before changes to the package.

@scd31 scd31 changed the title add wsjtz wsjtz: init at 2.7.0-rc7-1.43 Jan 7, 2025
@scd31
Copy link
Author

scd31 commented Jan 7, 2025

@GaetanLepage The commit is already named that but I updated the PR name

@l1npengtul Are they not already in this order? My git log shows the maintainer commit before the package commit

@l1npengtul
Copy link
Contributor

@l1npengtul Are they not already in this order? My git log shows the maintainer commit before the package commit

They are in the right order. Apologies, its been a long day >.<

GaetanLepage
GaetanLepage reviewed Jan 7, 2025
View reviewed changes
pkgs/by-name/ws/wsjtz/package.nix Outdated
Comment on lines 21 to 31
meta = with lib; {
description = "WSJT-X fork, primarily focused on automation and enhanced functionality";
homepage = "https://sourceforge.net/projects/wsjt-z/";
license = lib.licenses.gpl3;
platforms = platforms.linux;
maintainers = with maintainers; [
scd31
];
mainProgram = "wsjtz";
};
})
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
meta = with lib; {
description = "WSJT-X fork, primarily focused on automation and enhanced functionality";
homepage = "https://sourceforge.net/projects/wsjt-z/";
license = lib.licenses.gpl3;
platforms = platforms.linux;
maintainers = with maintainers; [
scd31
];
mainProgram = "wsjtz";
};
})
meta = {
description = "WSJT-X fork, primarily focused on automation and enhanced functionality";
homepage = "https://sourceforge.net/projects/wsjt-z/";
license = lib.licenses.gpl3;
platforms = lib.platforms.linux;
maintainers = with lib.maintainers; [
scd31
];
mainProgram = "wsjtz";
};
})

The with lib; pattern is discouraged.

Copy link
Author

Choose a reason for hiding this comment

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

Fixed :3

@l1npengtul
Copy link
Contributor

1 package built:
wsjtz

nixpkgs-review result

Generated using nixpkgs-review.

Command: nixpkgs-review

x86_64-linux

✅ 1 package built:
  • wsjtz

While it builds, the main wsjtz binary segfaults. I'm assuming this is an issue with the nix-shell, and not with the package.

l1npengtul
l1npengtul approved these changes Jan 7, 2025
View reviewed changes
Copy link
Contributor

@l1npengtul l1npengtul left a comment

Choose a reason for hiding this comment

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

Yep, pasted the package into my personal nixcfg and it builds and launches fine. LGTM!

@l1npengtul l1npengtul added the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Jan 7, 2025
GaetanLepage
GaetanLepage approved these changes Jan 7, 2025
View reviewed changes
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.

LGTM !

@GaetanLepage
Copy link
Contributor

nixpkgs-review result

Generated using nixpkgs-review.

Command: nixpkgs-review pr 367971

x86_64-linux

✅ 1 package built:
  • wsjtz

aarch64-linux

✅ 1 package built:
  • wsjtz

@GaetanLepage GaetanLepage requested a review from drupol January 7, 2025 22:44
drupol
drupol reviewed Jan 7, 2025
View reviewed changes
pkgs/by-name/ws/wsjtz/package.nix
lib,
}:

wsjtx.overrideAttrs (old: rec {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why the rec?

Copy link
Author

Choose a reason for hiding this comment

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

Don't I need it because I'm defining version and using it in the same block?

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh never mind.

Copy link
Author

Choose a reason for hiding this comment

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

No worries :3 I'm super new to nix so I wanted to make sure I wasn't missing something

@GaetanLepage GaetanLepage merged commit 0400660 into NixOS:master Jan 7, 2025
24 of 28 checks passed
@scd31 scd31 deleted the add-wsjtz branch January 7, 2025 23:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@FliegendeWurst FliegendeWurst FliegendeWurst left review comments

@l1npengtul l1npengtul l1npengtul approved these changes

@GaetanLepage GaetanLepage GaetanLepage approved these changes

@drupol drupol drupol approved these changes

Assignees
No one assigned
Labels
8.has: maintainer-list (update) This PR changes `maintainers/maintainer-list.nix` 8.has: package (new) This PR adds a new package 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 11.by: package-maintainer This PR was created by the maintainer of the package it changes 12.approvals: 1 This PR was reviewed and approved by one reputable person 12. first-time contribution This PR is the author's first one; please be gentle!
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@scd31 @liberodark @GaetanLepage @l1npengtul @drupol @FliegendeWurst @NixOSInfra