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

master_me: init at 1.2.0 #226373

Merged
merged 1 commit into from Jul 13, 2023
Merged

master_me: init at 1.2.0 #226373

merged 1 commit into from Jul 13, 2023

Conversation

magnetophon
Copy link
Member

@magnetophon magnetophon commented Apr 16, 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
  • Fits CONTRIBUTING.md.

@magnetophon
Copy link
Member Author

There seems to be a failing test https://github.com/NixOS/nixpkgs/pull/226373/checks?check_run_id=12776391277 caused by a too old macos version https://developer.apple.com/documentation/appkit/nspasteboardtypeowner
Is that correct?
Any suggestions?

@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-ready-for-review/3032/2276

@Janik-Haag
Copy link
Member

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

1 package built:
  • master_me

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

Can't really help with the macos part don't own a mac. Binary works fine under wayland/sway.

Comment on lines 22 to 26
patchShebangs ./dpf/utils/
substituteInPlace ./dpf/utils/res2c.py \
--replace '/usr/bin/env python3' ${python3.interpreter}
Copy link
Member

Choose a reason for hiding this comment

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

patchShebangs is not working if it can't find python in any of the inputs

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

this is still open and not good practise

pkgs/applications/audio/master_me/default.nix Outdated Show resolved Hide resolved
pkgs/applications/audio/master_me/default.nix Outdated Show resolved Hide resolved
@magnetophon
Copy link
Member Author

Thanks for the feedback, all done!

@SuperSandro2000
Copy link
Member

Thanks for the feedback, all done!

Did you do a wrong force push by accident? It seems like you reverted somet things again.

@magnetophon
Copy link
Member Author

Did you do a wrong force push by accident? It seems like you reverted somet things again.

I found only one force push on this page, from 2023-05-31.

I'm not sure where I can find the commit that has

broken = stdenv.isDarwin; # error: no type or protocol named 'NSPasteboardType'

Does it contain any other changes I should re-apply?

Copy link
Contributor

@PowerUser64 PowerUser64 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! I've been using this package for the last month and it has been working well. The changes to buildInputs suggested by @SuperSandro2000 seem like they would be good to add, but everything works the way it is now.

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.

LGTM

Copy link
Member

@SuperSandro2000 SuperSandro2000 left a comment

Choose a reason for hiding this comment

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

at least the shebang comment must be fixed

Comment on lines 22 to 26
patchShebangs ./dpf/utils/
substituteInPlace ./dpf/utils/res2c.py \
--replace '/usr/bin/env python3' ${python3.interpreter}
Copy link
Member

Choose a reason for hiding this comment

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

this is still open and not good practise

pkgs/applications/audio/master_me/default.nix Outdated Show resolved Hide resolved
@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-ready-for-review/1016/63

@Janik-Haag
Copy link
Member

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

1 package built:
  • master_me

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.

This looks good execept for the formatting nitpick. Builds fine and works as expected

pkgs/applications/audio/master_me/default.nix Outdated Show resolved Hide resolved
@SuperSandro2000 SuperSandro2000 merged commit b1aaa2a into NixOS:master Jul 13, 2023
21 checks passed
@magnetophon magnetophon deleted the master_me branch July 13, 2023 20:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants