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

starfetch: init at 0.0.2 #167541

Merged
merged 1 commit into from
Jun 26, 2022
Merged

starfetch: init at 0.0.2 #167541

merged 1 commit into from
Jun 26, 2022

Conversation

auroraanna
Copy link
Contributor

@auroraanna auroraanna commented Apr 6, 2022

Description of changes

Add the starfetch package

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.

@auroraanna
Copy link
Contributor Author

auroraanna commented Apr 6, 2022

Whoops, my automatic nix-fmt on save did a lot of unwanted things to pkgs/top-level/all-packages.nix.

@auroraanna
Copy link
Contributor Author

It's fixed now.

@auroraanna
Copy link
Contributor Author

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

1 package built:
  • starfetch

@auroraanna
Copy link
Contributor Author

Added meta.homepage.

@auroraanna
Copy link
Contributor Author

Updated to newest commit.

pkgs/tools/misc/starfetch/default.nix Outdated Show resolved Hide resolved
pkgs/tools/misc/starfetch/default.nix Show resolved Hide resolved
pkgs/tools/misc/starfetch/default.nix Show resolved Hide resolved

stdenv.mkDerivation rec {
pname = "starfetch";
version = "unstable-2022-04-06";
Copy link
Member

Choose a reason for hiding this comment

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

Since the maintainer is interested in this being packaged id advise they tag a version 0.0.1
People are more likely to package a release than a commit

Haruno19/starfetch#1

@auroraanna
Copy link
Contributor Author

currently blocked by
Haruno19/starfetch#10

@auroraanna
Copy link
Contributor Author

Everything should be fixed now. I patched the source code to have the nix store paths.

};

postPatch = ''
substituteInPlace ./src/starfetch.cpp --replace /usr/local/starfetch $out
Copy link
Member

Choose a reason for hiding this comment

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

You should use makeFlags instead of patching

https://github.com/Haruno19/starfetch/blob/main/makefile#L4

Copy link
Contributor Author

@auroraanna auroraanna Apr 16, 2022

Choose a reason for hiding this comment

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

What does makeFlags have to do with the paths inside the source code? If starship.cpp is hardcoded to /usr/local how is makeFlags gonna change that?

Copy link
Member

Choose a reason for hiding this comment

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

Ah its the cpp files.

If its the res dir it shouldn't be at the root of the $out

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 asked the developer about that too: Haruno19/starfetch#2

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should we tell him to put it somewhere in /usr/share?

Copy link
Member

Choose a reason for hiding this comment

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

🤷 not sure. I haven't even looked deep enough to work out if these resources are static or should be updated etc so I couldn't give an accurate answer

Choose a reason for hiding this comment

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

Should we tell him to put it somewhere in /usr/share?

So, would it be better to use /usr/share/starfetch rather than /usr/local/starfetch to store resource files (txt and json files)?
There's no problem in changing that, but may I ask why?

Copy link
Member

@06kellyjac 06kellyjac Apr 19, 2022

Choose a reason for hiding this comment

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

I'd imagine the argument is:

/usr/share : Architecture-independent data

https://refspecs.linuxfoundation.org/FHS_3.0/fhs/ch04s11.html

Within /usr/local you could do /usr/local/share/starfetch but probably not /usr/local/starfetch

No other directories, except those listed below, may be in /usr/local after first installing a FHS-compliant system.

see the directories section here

https://refspecs.linuxfoundation.org/FHS_3.0/fhs/ch04s09.html


So TL;DR /usr/share/starfetch or /usr/local/share/starfetch would be your options
And then I personally wouldn't add a res subdir since your res dir already is split nicely into 2 files and a dir

constellations/
help_message.txt
template

Choose a reason for hiding this comment

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

Thank you for the exhaustive reply.
I'll implement this changes as soon as possible, and then add a new release.

Choose a reason for hiding this comment

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

Release 0.0.2 is now out. Files are now stored into /usr/local/share/starfetch/ and the res/ subdir has been removed.

@auroraanna auroraanna changed the title starfetch: init at unstable-2022-04-06 starfetch: init at 0.0.1 Apr 16, 2022
@auroraanna auroraanna changed the title starfetch: init at 0.0.1 starfetch: init at 0.0.2 Apr 19, 2022
@auroraanna
Copy link
Contributor Author

I don't know if it's possible to use the makefile's install section for installing but I figured it doesn't make sense anyway because there are some commands in the makefile that aren't needed with Nix.

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.

4 participants