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

neohtop: init at 1.1.1 #356826

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

neohtop: init at 1.1.1 #356826

wants to merge 1 commit into from

Conversation

HannesGitH
Copy link
Contributor

@HannesGitH HannesGitH commented Nov 17, 2024 •
edited
Loading

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.

@HannesGitH HannesGitH force-pushed the neohtop branch 2 times, most recently from ff162e7 to d7db841 Compare November 18, 2024 15:03
@HannesGitH HannesGitH marked this pull request as ready for review November 18, 2024 15:03
@HannesGitH HannesGitH force-pushed the neohtop branch 2 times, most recently from 3715bdb to 3bd3da6 Compare November 18, 2024 15:16
@HannesGitH HannesGitH marked this pull request as draft November 18, 2024 15:16
@HannesGitH HannesGitH marked this pull request as ready for review November 18, 2024 15:22
@HannesGitH
Copy link
Contributor Author

HannesGitH commented Nov 18, 2024 •
edited
Loading

@NixOS/nix-formatting the formatting check fails if formatted with
nix run '.#nixfmt' 'pkgs/by-name/ne/neohtop/package.nix'
but then recommands using exactly that to format, but it actually requires
nix run '.#nixfmt-rfc-style' 'pkgs/by-name/ne/neohtop/package.nix'

@HannesGitH HannesGitH mentioned this pull request Nov 18, 2024
Closed
12 tasks
@ambroisie
Copy link
Contributor

A new fetcher has been introduced and its usage is encouraged: #349360.

@HannesGitH
Copy link
Contributor Author

A new fetcher has been introduced and its usage is encouraged: #349360.

the docs say to use that new fetcher only if cargo lock has git dependencies:

Exception: If the application has cargo git dependencies, the cargoHash approach will not work by default. In this case, you can set useFetchCargoVendor = true to use an improved fetcher that supports handling git dependencies.

but if you say I should use that.. does simply adding useFetchCargoVendor = true; do the trick, or do i need to something else? @ambroisie

@ambroisie
Copy link
Contributor

Hmm, my understanding was that it avoiding us vendoring the Cargo.lock was a plus in all cases.

I'll defer to more knowledgeable Rust maintainers to weight in the.

JohnRTitor
JohnRTitor reviewed Nov 19, 2024
View reviewed changes
pkgs/by-name/ne/neohtop/package.nix Outdated
inherit src;
hash = "sha256-6FWowWVVeU7U5aM4WGVWv0WmauLv/F4mtSdjNeeF6eQ=";
};
cargoHash = "";
Copy link
Contributor

@JohnRTitor JohnRTitor Nov 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Why not use cargoHash and get rid of the lockfile?

Copy link
Contributor Author

@HannesGitH HannesGitH Nov 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

because this does not work (cargo cannot find reqwest and the build fails) I think its related to:
https://github.com/HannesGitH/nixpkgs/blob/c43b2b1ea412767dd01e4354f4ca05ff942df4e4/pkgs/by-name/su/surrealist/package.nix#L52-L57

Copy link
Contributor Author

Choose a reason for hiding this comment

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

also see #354565 (comment)

FliegendeWurst
FliegendeWurst requested changes Nov 19, 2024
View reviewed changes
Copy link
Member

@FliegendeWurst FliegendeWurst left a comment

Choose a reason for hiding this comment

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

A meta attribute is required

FliegendeWurst
FliegendeWurst requested changes Nov 20, 2024
View reviewed changes
pkgs/by-name/ne/neohtop/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/ne/neohtop/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/ne/neohtop/package.nix Outdated Show resolved Hide resolved
@ofborg ofborg bot added the 11.by: package-maintainer This PR was created by the maintainer of the package it changes label Nov 21, 2024
FliegendeWurst
FliegendeWurst requested changes Nov 21, 2024
View reviewed changes
Copy link
Member

@FliegendeWurst FliegendeWurst left a comment

Choose a reason for hiding this comment

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

I think you need to include openssl in buildInputs

@ghost
Copy link

ghost commented Nov 21, 2024

#354007

@HannesGitH
Copy link
Contributor Author

I think you need to include openssl in buildInputs

why? it builds and runs as it currently is

@FliegendeWurst
Copy link
Member

why? it builds and runs as it currently is

Not on Linux (see ofborg)

@HannesGitH
Copy link
Contributor Author

HannesGitH commented Nov 21, 2024 •
edited
Loading

why? it builds and runs as it currently is

Not on Linux (see ofborg)

aight i'll quickly add openssl
is added now ✅
lets see

@FliegendeWurst
Copy link
Member

It is a little hidden because it will mark a build failure as "neutral".

pluiedev
pluiedev suggested changes Nov 21, 2024
View reviewed changes
pkgs/by-name/ne/neohtop/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/ne/neohtop/package.nix Show resolved Hide resolved
pkgs/by-name/ne/neohtop/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/ne/neohtop/package.nix Show resolved Hide resolved
pkgs/by-name/ne/neohtop/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/ne/neohtop/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/ne/neohtop/package.nix Outdated
mkdir -p $out/bin
mv ${path}/macos $out/Applications
echo "#!${zsh}/bin/zsh" >> $out/bin/${name}
echo "open -a $out/Applications/${name}.app" >> $out/bin/${name}
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the normal way to do this is simply to copy the entire .app folder to $out(maybe share)?

pkgs/by-name/ne/neohtop/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/ne/neohtop/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/ne/neohtop/package.nix Outdated Show resolved Hide resolved
@HannesGitH HannesGitH marked this pull request as draft November 21, 2024 20:49
@HannesGitH
Copy link
Contributor Author

thanks for your feedback, ill hopefully be able to integrate that tomorrow!

@FliegendeWurst FliegendeWurst added the awaiting_changes (old Marvin label, do not use) label Nov 24, 2024
@SigmaSquadron SigmaSquadron removed the awaiting_changes (old Marvin label, do not use) label Jan 5, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JohnRTitor JohnRTitor JohnRTitor left review comments

@pluiedev pluiedev pluiedev requested changes

@FliegendeWurst FliegendeWurst FliegendeWurst requested changes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
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
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@HannesGitH @ambroisie @FliegendeWurst @pluiedev @JohnRTitor @SigmaSquadron