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

tiatracker: init at 1.3 #158645

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

tiatracker: init at 1.3 #158645

wants to merge 2 commits into from

Conversation

fgaz
Copy link
Member

@fgaz fgaz commented Feb 8, 2022

Motivation for this change

#81815 cc @OPNA2608

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/)
  • 22.05 Release Notes (or backporting 21.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
Contributor

@OPNA2608 OPNA2608 left a comment

Choose a reason for hiding this comment

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

Code looks okay. Build passes & binary works on Linux, but fails on Darwin.

Looks like the QMake script was never tested on Darwin. I haven't tested it locally but try this:

postPatch = ''
  substituteInPlace TIATracker.pro \
    --replace 'linux:' 'unix:'
'';

If that doesn't fix the build on CI I'll look into it on my machine.


As a note, do you know about this fork? Haven't tested it myself but it seems like it has some nice additions.

Bildschirmfoto von 2022-02-10 00-14-20

@fgaz
Copy link
Member Author

fgaz commented Feb 10, 2022

As a note, do you know about this fork? Haven't tested it myself but it seems like it has some nice additions.

Ooh that's nice! Do you think we should package that instead, or do both?

@OPNA2608
Copy link
Contributor

Fixed all the Darwin stuff in fgaz#3, builds & works on my machine now.

Do you think we should package that instead, or do both?

I would be in favour of doing both just cuz "why not?". But I don't have much experience with using trackers or the TIA so I can't judge how crucial these changes are (except undo/redo, which is obviously an improvement).

@fgaz
Copy link
Member Author

fgaz commented Feb 12, 2022

I would be in favour of doing both just cuz "why not?"

That's pretty much in the spirit of nixpkgs, and I agree :D
I'll add it to the list

@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-already-reviewed/2617/471


meta = with lib; {
inherit description;
inherit (src.meta) homepage;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
inherit (src.meta) homepage;
homepage = "https://bitbucket.org/kylearan/tiatracker";

'';

meta = with lib; {
inherit description;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
inherit description;
description = "Music tracker for making Atari VCS 2600 music";

SDL2
];

desktopItems = lib.optionals stdenv.hostPlatform.isLinux [ desktopItem ];
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
desktopItems = lib.optionals stdenv.hostPlatform.isLinux [ desktopItem ];
desktopItems = [
(makeDesktopItem {
name = "TIATracker";
desktopName = "TIATracker";
comment = meta.description;
exec = "TIATracker";
icon = "tiatracker";
type = "Application";
categories = "AudioVideo;AudioVideoEditing;";
extraEntries = "Keywords=tracker;music;";
})
];

Comment on lines +13 to +26
let
description = "Music tracker for making Atari VCS 2600 music";
desktopItem = makeDesktopItem {
name = "TIATracker";
desktopName = "TIATracker";
comment = description;
exec = "TIATracker";
icon = "tiatracker";
type = "Application";
categories = "AudioVideo;AudioVideoEditing;";
extraEntries = "Keywords=tracker;music;";
};

in mkDerivation rec {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
let
description = "Music tracker for making Atari VCS 2600 music";
desktopItem = makeDesktopItem {
name = "TIATracker";
desktopName = "TIATracker";
comment = description;
exec = "TIATracker";
icon = "tiatracker";
type = "Application";
categories = "AudioVideo;AudioVideoEditing;";
extraEntries = "Keywords=tracker;music;";
};
in mkDerivation rec {
mkDerivation rec {

@onny onny marked this pull request as draft September 5, 2022 10:16
@wegank wegank added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Mar 19, 2024
@wegank wegank added the 2.status: merge conflict This PR has merge conflicts with the target branch label Nov 9, 2024
@stale stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Nov 9, 2024
@wegank wegank added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jan 1, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2.status: merge conflict This PR has merge conflicts with the target branch 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md 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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants