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

Revert "river: refactor" #121405

Merged
merged 1 commit into from
May 2, 2021
Merged

Revert "river: refactor" #121405

merged 1 commit into from
May 2, 2021

Conversation

heavyrain266
Copy link
Contributor

Reverts #121357

Why revert?

  • Originally packages in header are sorted by phases for easier dependency management (by libs etc.)
  • Run hooks in prebuild gives me segfault locally (tested on NixOS and kiss linux with nix)
  • No needed for super long description about design goals copied straight from project's readmie
  • Derivation was built twice because of zig build in preBuild phase
  • Looks like previous commiter doesn't know anything about zig and it's build system

@r-rmcgibbo
Copy link

r-rmcgibbo commented May 1, 2021

Result of nixpkgs-review pr 121405 at 8776e9a run on x86_64-linux 1

1 package built successfully:
  • river
1 suggestion:
  • warning: missing-phase-hooks

    installPhase should probably contain runHook preInstall and runHook postInstall.

    Near pkgs/applications/window-managers/river/default.nix:26:3:

       |
    26 |   installPhase = ''
       |   ^
    

Result of nixpkgs-review pr 121405 at 8776e9a run on aarch64-linux 1

1 package built successfully:
  • river

@jonringer
Copy link
Contributor

cc @AndersonTorres

'';

nativeBuildInputs = [ zig wayland scdoc pkg-config ];

installFlags = [ "DESTDIR=$(out)" ];
Copy link
Member

Choose a reason for hiding this comment

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

It should not be needed. DESTDIR has the same semantics of autotools' DESTDIR variable; therefore, it is not useful for Nix.

DESTDIR is useful for classic package managers/build scripts like Slackbuilds or PKGBUILDs.

@AndersonTorres
Copy link
Member

It will be long...

Derivation was built twice because of zig build in preBuild phase

That was my fault.

I just followed the AUR script here. It repeats the command adding the 'install' argument in the second invocation.

It looks like the same principle of calling make && make install from classic projects. Following the idea that make does not rebuild targets already built, it appeared that Zig would do the same, not rebuilding already built targets.

Also, it looked strange conflating the build and install phases in a single one. This way, the AUR script looks plausible.

But I should have looked the build logs. Indeed it rebuilds the whole tree. My fault, my maximum fault.

On the other hand, there is another way to make the expression more nixy: use phases = [ "unpackPhase" "installPhase" ];.

Run hooks in prebuild gives me segfault locally (tested on NixOS and kiss linux with nix)

Hook on a prebuild? preBuild is a hook. I suppose you meant to say using runHook is creating segfaults; this is unexpected at least. Maybe a bug on nixpkgs or something?

On the other hand,, adding runHook is mandatory - look at the @r-rmcgibbo suggestion above.

@AndersonTorres
Copy link
Member

AndersonTorres commented May 2, 2021

No needed for super long description about design goals copied straight from project's readmie

Here I will differ.

A longDescription that just repeats the short description, without adding substantial information to it, is not useful.

If a long description is to be included, there is no problem in copying sentences directly from upstream README. Indeed, this is the regular practice among nixpkgs expressions, and it conveys a useful introduction about the package ("minimalist, stacked", "rivertile and riverctl" etc.).

Also, this is not superlong at all - it ocuppies less than 15% of the file.

Else, just remove longDescription completely. A mere copy-paste of the first line of upstream README that has no substantial difference compared with the short description is useless.

@AndersonTorres
Copy link
Member

For now I will revert.

@AndersonTorres AndersonTorres merged commit bebfaab into NixOS:master May 2, 2021
@heavyrain266
Copy link
Contributor Author

I will just add runHook after next update, for now just DESTDIR creates folder with manpages and 3 binaries in river derivation folder, without it won't work and that one mentioned in zig's github is included in nightly build where we are using 0.7.1 which is stable and miss a lot of features

@AndersonTorres
Copy link
Member

AndersonTorres commented May 2, 2021

for now just DESTDIR creates folder with manpages and 3 binaries in river derivation folder

There is no need to set DESTDIR:

result/
result/etc
result/etc/river
result/etc/river/init
result/bin
result/bin/river
result/bin/riverctl
result/bin/rivertile
result/share
result/share/man
result/share/man/man1
result/share/man/man1/river.1.gz
result/share/man/man1/riverctl.1.gz
result/share/man/man1/rivertile.1.gz
result/share/bash-completion
result/share/bash-completion/completions
result/share/bash-completion/completions/riverctl
result/share/zsh
result/share/zsh/site-functions
result/share/zsh/site-functions/_riverctl
result/share/fish
result/share/fish/vendor_completions.d
result/share/fish/vendor_completions.d/riverctl.fish

and that one mentioned in zig's github is included in nightly build

2019, release 0.5.0.

ziglang/zig@d6d0bb0

we are using 0.7.1 which is stable and miss a lot of features

The readmie recommends 0.7.1, not the master branch.

Nothing of you said makes sense.

@heavyrain266
Copy link
Contributor Author

None of your changes make sense, same as what you said comparing derivations to completelly different PKGBUILDs....

@jonringer
Copy link
Contributor

On the other hand, there is another way to make the expression more nixy: use phases = [ "unpackPhase" "installPhase" ];.

specific phases are discouraged, as phases such as "fixupPhase" still help with nixification (patching shebangs, etc). or at the very least, you should be including patchPhase and fixupPhase.

@AndersonTorres
Copy link
Member

AndersonTorres commented May 2, 2021

comparing derivations to completelly different PKGBUILDs....

"Completely different"?

Nixpkgs is not an "esoteric" system like Windows or OS/2. It is a package manager that expects a typical GNU environment and executes Bash scripts to perform its tasks. Hey, you quoted KISS Linux!

Things that affect other package managers in general affect Nixpkgs in a similar manner: deprecation of unmaintained software, unstable upstream dependencies, patches &c. Indeed, you can look at the source code of many derivations and notice that they use patches from Arch, Debian, and even Gentoo. Such things would be impossible in "completely different" environments.

You assertion of "completely different" is unwarranted at least.

@heavyrain266 heavyrain266 deleted the revert-121357-new-river branch May 7, 2021 16:42
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