Skip to content

bash-env-json: fix path issue#358950

Merged
JohnRTitor merged 1 commit intoNixOS:masterfrom
jaredmontoya:bash-env-json-fix
Dec 15, 2024
Merged

bash-env-json: fix path issue#358950
JohnRTitor merged 1 commit intoNixOS:masterfrom
jaredmontoya:bash-env-json-fix

Conversation

@jaredmontoya
Copy link
Copy Markdown
Contributor

@jaredmontoya jaredmontoya commented Nov 25, 2024

Things done

Fixed bash-env-json breaking if it tries to process a file that sets PATH.
This program cannot rely on PATH, therefore full paths must be used in this program all the time.
Upstream flake currently uses the same principle to build the package.

  • 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.

@jaredmontoya
Copy link
Copy Markdown
Contributor Author

@Aleksanaa can you please read this issue through and tell us how to go about this package in the long run?
tesujimath/bash-env-json#14

I want to know if there are any other nixpkgs packages that use nix code from the repo they are downloading to build the package and if it is even possible.

And if it is, is it allowed due to the fact that nix reviewers will have to review outside code that is imported before merging the updated derivation.

I think I already know the answer but I don't think I should be the one to say the final verdict to the maintainer of the program because I am just a package maintainer in nixpkgs who doesn't know all the rules, not an authority.

@Aleksanaa
Copy link
Copy Markdown
Member

There's actually a tool called resholve here: https://github.com/NixOS/nixpkgs/tree/nixos-unstable/pkgs/development/misc/resholve

it correctly identifies executables in scripts and replaces them with those provided in packages, and fails if the provided list isn't sufficient. However, it depends on Python2 thus is discouraged to use. But I still recommend you use it here, if possible.

@Aleksanaa
Copy link
Copy Markdown
Member

And if it is, is it allowed due to the fact that nix reviewers will have to review outside code that is imported before merging the updated derivation.

This is probably unrealistic. We will read the diff, but this does not mean that we can always fully understand them...

@Aleksanaa
Copy link
Copy Markdown
Member

And we have no responsibility to enforce a standard in other people's projects, which is needed in Nixpkgs from time to time. To put it bluntly, the writing format of most out-of-tree derivation is completely substandard (although some in-tree derivation is also like this)

@ofborg ofborg bot added 11.by: package-maintainer This PR was created by a maintainer of all the package it changes. 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-darwin: 1 This PR causes 1 package to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. 10.rebuild-linux: 1 This PR causes 1 package to rebuild on Linux. labels Nov 26, 2024
@jaredmontoya
Copy link
Copy Markdown
Contributor Author

jaredmontoya commented Nov 26, 2024

@Aleksanaa Thank you for your explanation.

The main concern for us is, that if the bash-env-json maintainer adds another dependency to the script it will also have to be added to the derivation along with the update and it's worse than just simply changing the version number and hash in the derivation.

I didn't know about resholve and it sounds great except for the fact that it uses python2, but I think it won't exactly solve the issue because it still requires changes to inputs and package call parameters if new dependencies are introduced.

Also resholve failed with if ! source "$_path" >/dev/null 2>&4; then Can't resolve dynamic argument in 'source' on bash-env-json so it's out of the question if this error is not a result of my incompetency with resholve
here is what I tested it with:

{
  resholve,
  bash,
  lib,
  fetchFromGitHub,
  coreutils,
  gnused,
  jq,
  nix-update-script,
}:

resholve.mkDerivation rec {
  pname = "bash-env-json";
  version = "0.9.2";

  src = fetchFromGitHub {
    owner = "tesujimath";
    repo = "bash-env-json";
    rev = version;
    hash = "sha256-EYro4pMILnQFpXpFjdzSDuudhqC2EvysYMUmIOvesgo=";
  };

  installPhase = ''
    runHook preInstall

    install -Dm755 bash-env-json -t $out/bin

    runHook postInstall
  '';

  solutions = {
    default = {
      scripts = [ "bin/bash-env-json" ];
      interpreter = lib.getExe bash;
      inputs = [
        coreutils
        gnused
        jq
      ];
    };
  };

  passthru.updateScript = nix-update-script { };

  meta = {
    description = "Export Bash environment as JSON for import into modern shells like Elvish and Nushell";
    homepage = "https://github.com/tesujimath/bash-env-json";
    mainProgram = "bash-env-json";
    license = lib.licenses.mit;
    maintainers = with lib.maintainers; [ jaredmontoya ];
    platforms = lib.platforms.all;
  };
}

But as the docs say:

Fair warning: resholve does not aspire to resolving all valid Shell scripts. It depends on the OSH/Oil parser, which aims to support most (but not all) Bash. resholve aims to be a ~90% sort of solution.`

so it's expected.

I think I will just have to check a small section of the source code for new required dependencies before updating the package and add appropriate substituteInPlace which is not a big deal.

@jaredmontoya
Copy link
Copy Markdown
Contributor Author

jaredmontoya commented Dec 7, 2024

@Aleksanaa what is happening with https://github.com/NixOS/nixpkgs/pull/358950/checks?check_run_id=33519605117

Why does it take more than 13 days?

@jaredmontoya
Copy link
Copy Markdown
Contributor Author

jaredmontoya commented Dec 9, 2024

Finally, it's done.
Please merge.

@jaredmontoya
Copy link
Copy Markdown
Contributor Author

jaredmontoya commented Dec 15, 2024

@teto Can you merge this please? It will help to improve the nushell home-manager module.

@JohnRTitor JohnRTitor merged commit 9f1bd52 into NixOS:master Dec 15, 2024
@jaredmontoya jaredmontoya deleted the bash-env-json-fix branch January 10, 2025 11:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-darwin: 1 This PR causes 1 package to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. 10.rebuild-linux: 1 This PR causes 1 package to rebuild on Linux. 11.by: package-maintainer This PR was created by a maintainer of all the package it changes.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants