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

ttaenc: init at 3.4.1 #238757

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

ttaenc: init at 3.4.1 #238757

wants to merge 2 commits into from

Conversation

natsukagami
Copy link
Contributor

Description of changes

ttaenc homepage: https://sourceforge.net/projects/tta/ and http://tta.tausoft.org/
build adapted from the AUR package https://aur.archlinux.org/packages/ttaenc

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.11 Release Notes (or backporting 23.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.

@natsukagami natsukagami force-pushed the ttaenc-init branch 2 times, most recently from 7a577a5 to 12cb7e2 Compare June 20, 2023 13:17
@piegamesde
Copy link
Member

Please format the commit message as "maintainers: add natsukagami"

@piegamesde piegamesde removed the request for review from Janik-Haag December 29, 2023 13:45
Janik-Haag
Janik-Haag reviewed Dec 31, 2023
View reviewed changes
Copy link
Member

@Janik-Haag Janik-Haag 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 to me other then the two comments

pkgs/applications/audio/ttaenc/default.nix Outdated Show resolved Hide resolved
pkgs/applications/audio/ttaenc/default.nix Outdated Show resolved Hide resolved
@Janik-Haag
Copy link
Member

oh and you should add badPlatforms = lib.platforms.darwin; or broken = stdenv.isDarwin; to meta depending on if the package has darwin support but is broken or if it is unsupported and thus a bad platform.

@piegamesde piegamesde removed their request for review December 31, 2023 13:22
@wegank wegank added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jul 4, 2024
@natsukagami natsukagami force-pushed the ttaenc-init branch 2 times, most recently from e45118b to 171edcd Compare October 3, 2024 14:26
@Janik-Haag
Copy link
Member

Hey, I left nixpkgs some time ago and thus won't be reviewing this PR anymore. #323901

@stale stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Oct 4, 2024
@Janik-Haag Janik-Haag removed their request for review October 4, 2024 00:36
Frontear
Frontear reviewed Oct 4, 2024
View reviewed changes
pkgs/by-name/tt/ttaenc/package.nix

preBuild = ''
# From the Makefile, with `-msse` removed, since we have those on by x86_64 by default.
makeFlagsArray+=(CFLAGS="-Wall -O3 -fomit-frame-pointer -funroll-loops -fforce-addr -falign-functions=4")
Copy link
Member

Choose a reason for hiding this comment

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

just out of curiosity, is there a reason you used this here instead of just adding to the makeFlags attribute? does it operate semantically differently?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this has something to do with my CFLAGS having spaces inside. I wasn't able to figure out how to do that with makeFlags, and saw some other nixpkgs derivation doing this instead, so I copied

Frontear
Frontear reviewed Oct 4, 2024
View reviewed changes
pkgs/by-name/tt/ttaenc/package.nix
postInstall = ''
# Copy docs
install -dm755 "$out/share/doc/${finalAttrs.name}"
install -m644 "ChangeLog-${finalAttrs.version}" README "$out/share/doc/${finalAttrs.name}"
Copy link
Member

Choose a reason for hiding this comment

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

nit: these two could be merged into one install -Dm755 -t $out/share/doc/${finalAttrs.name} ChangeLog-${finalAttrs.version} README

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Didn't know that, looks cool! Would it keep the latter files at 0644 permission?

Copy link
Member

Choose a reason for hiding this comment

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

Good point actually. The default umask should enforce 644 but it isn't explicit. In that regards yours is better.

Frontear
Frontear suggested changes Oct 4, 2024
View reviewed changes
Copy link
Member

@Frontear Frontear left a comment •
edited
Loading

Choose a reason for hiding this comment

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

main concern is the merge conflict, everything else looks good though. tested the binary using nixpkgs-review pr --checkout commit 238757 and it works.

also if you havent already, running nixfmt on this would be nice (though it already seems formatted)

Frontear
Frontear suggested changes Oct 4, 2024
View reviewed changes
Copy link
Member

@Frontear Frontear left a comment

Choose a reason for hiding this comment

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

please fix the merge conflict, then lgtm!

maintainers/maintainer-list.nix Show resolved Hide resolved
@natsukagami
maintainers: update natsukagami
11b62e6 
With email matching GPG key, and other info
@Frontear
Copy link
Member

Frontear commented Oct 4, 2024

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

1 package built:
  • ttaenc

@natsukagami
Copy link
Contributor Author

Some patches introduced to make it compile on darwin, tested by a friend

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Janik-Haag Janik-Haag Janik-Haag left review comments

@Frontear Frontear Frontear approved these changes

Assignees
No one assigned
Labels
8.has: maintainer-list (update) 8.has: package (new) 10.rebuild-darwin: 1-10 10.rebuild-darwin: 1 10.rebuild-linux: 1-10 10.rebuild-linux: 1 11.by: package-maintainer
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@natsukagami @piegamesde @Janik-Haag @Frontear @wegank