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

far2l: 2019-12-14 -> 2020-12-30 #108750

Merged
merged 2 commits into from Feb 10, 2021
Merged

far2l: 2019-12-14 -> 2020-12-30 #108750

merged 2 commits into from Feb 10, 2021

Conversation

hypersw
Copy link
Contributor

@hypersw hypersw commented Jan 8, 2021

Motivation for this change

far2l were not able to run on a GUI-less NixOS installation, just exited silently. Upon a short diagnostic together with folks from far2l, ruled out that we had a version bundled which didn't have the tty-compat code yet.

Things done

Updated to a fresh master revision, fixed compilation, checked that it runs and appears operational.
The app build has some packages optional, needed to enable operations of plugins like network share browsers, added them to the build.

  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)

  • Built on platform(s)

    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests) -- found no relevant tests

  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip" -- the command messed up in patches, but I believe no packages depend on far2l, based on repo search

  • Tested execution of all binary files (usually in ./result/bin/) -- tested running some of the plugins, incl which were just added

  • Determined the impact on package closure size (by running nix path-info -S before and after)

  • Ensured that relevant documentation is up to date -- I think there're no docs outside package

  • Fits CONTRIBUTING.md.

@hypersw
Copy link
Contributor Author

hypersw commented Jan 8, 2021

anonymous function at /var/lib/ofborg/checkout/repo/38dca4e3aa6bca43ea96d2fcc04e8229/mr-est/ofborg-evaluator-3/pkgs/applications/misc/far2l/default.nix:1:1 called without required argument 'smbclient', at /var/lib/ofborg/checkout/repo/38dca4e3aa6bca43ea96d2fcc04e8229/mr-est/ofborg-evaluator-3/lib/customisation.nix:69:16
Means it's using a package alias, and should reference it by the canonical name instead? Like, smbclient = samba; # added 2018-04-25 in pkgs/top-level/aliases.nix:574 ?

@SuperSandro2000 SuperSandro2000 changed the title far2l far2l: 2019-12-14 -> 2020-12-30 Jan 8, 2021
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.

Please squash the commits.

pkgs/applications/misc/far2l/default.nix Outdated Show resolved Hide resolved
pkgs/applications/misc/far2l/default.nix Outdated Show resolved Hide resolved
pkgs/applications/misc/far2l/default.nix Outdated Show resolved Hide resolved
pkgs/applications/misc/far2l/default.nix Outdated Show resolved Hide resolved
@hypersw
Copy link
Contributor Author

hypersw commented Jan 8, 2021

Please squash the commits.

I've squashed work in progress as much as I thought reasonable. The remaining commits are independent changes I'd rather see individually in the history if this were on my project; might not be the same reasoning for nixos, please advise. They're not about the update as it is but the small off-things I noticed during the update.

@thiagokokada
Copy link
Contributor

Result of nixpkgs-review pr 108750 1

1 package built:
  • far2l

@SuperSandro2000
Copy link
Member

SuperSandro2000 commented Jan 10, 2021

The remaining commits are independent changes I'd rather see individually in the history if this were on my project; might not be the same reasoning for nixos, please advise. They're not about the update as it is but the small off-things I noticed during the update.

We usually just squash all those commits together to not explode the commit history. I don't think there is anything worth to keep here and I might just squash merge the PR.

@hypersw
Copy link
Contributor Author

hypersw commented Feb 7, 2021

@SuperSandro2000: Squashed together all extra improvements not related to merely advancing the version; squashmerge if this is still to much. Resolved all the other stuff.

pkgs/applications/misc/far2l/default.nix Outdated Show resolved Hide resolved
pkgs/applications/misc/far2l/default.nix Outdated Show resolved Hide resolved
pkgs/applications/misc/far2l/default.nix Outdated Show resolved Hide resolved
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.

3 little formatting changes after which it is good to be merged. I am providing a patch for darwin if anything fails.

Edit: Darwin does not build because the code base seems to be not compatible right now.

If you want you can remove the darwin specific code and mark it broken for darwin or just mark it broken.

pkgs/applications/misc/far2l/default.nix Outdated Show resolved Hide resolved
pkgs/applications/misc/far2l/default.nix Outdated Show resolved Hide resolved
pkgs/applications/misc/far2l/default.nix Outdated Show resolved Hide resolved
This change is required to enable running far2l on GUI-less linux, older version were bound to GTK+ and failed to start.
The colorer has been updated since in the far2l repo to a fresh version, so probably patching the schemes is not needed.
Added missing packages which are now needed to compile successfully.
Add more input packages, as required by optional features. Without them, build succeeds, but part of the functionality is not available.

Update package description to be closer to the oneliner in the upstream repo readme & FAR Manager official website, gets a bit more informative for general public.

Change license as requested by `thiagokokada` at NixOS#108750 (comment).
Checked with Far2l repo, and it just keeps a copy of the license without stating if it should be applied as "exact" or "at least".
Talked to the team, they say their main limiting license is coming from Wine, and Wine is "at least". So putting "at least".
But Wine has advanced their license since, so probably Far2l would also change the license soon, will keep track.
For the current moment, this is the best match.

Add to maintainers watchlist.

Compilation for Darwin broken in the codebase, disable explicitly.
@hypersw
Copy link
Contributor Author

hypersw commented Feb 10, 2021

@SuperSandro2000, thanks for your help!

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

4 participants