blesh: init at 2022-07-24 #181963
blesh: init at 2022-07-24 #181963
Conversation
|
@akinomyoga Pinging the creator of |
ofborg
bot
added
8.has: package (new)
There was a problem hiding this comment.
Thank you for creating a package of ble.sh!
There seems to be already a similar PR #160857, though it is for the devel version 0.4. You might find the comments therein useful.
btw, not bre.sh but ble.sh.
|
Maybe @SuperSandro2000 also have comments as @SuperSandro2000 seemed to have once attempted to use ble.sh with NixOS. |
|
Oh sorry for typo😅 I didn't noticed of #160857. I wish I had found it; it might sped up my work. The existence of |
|
This is a quick note for reviewers: I made this derivation first for the latest unstable ble.sh not v0.3.3, and downgraded it afterwards. v0.3.3 does not require git, but I left it on purpose to make it easier to upgrade. Those who want to use the latest version can override it with ease. |
pkgs/shells/bash/ble.sh/default.nix
Outdated
| name = "ble.sh"; | ||
| version = "0.3.3"; | ||
|
|
||
| nativeBuildInputs = [ git ]; |
There was a problem hiding this comment.
Why is git needed for a... line editor?
There was a problem hiding this comment.
Also, it should be after src.
There was a problem hiding this comment.
It's a build-time dependency.
Edit: The GNUmakefile internally calls git submodule update, but I guess we don't need it as far as we specify fetchSubmodules = true?
There was a problem hiding this comment.
On latest unstable ble.sh it is used to ensure submodule is fetched. v0.3.3 does not require it thou.
There was a problem hiding this comment.
After removing git, it is unable to be built anymore although fetchSubmodules = true. I'll leave this.
There was a problem hiding this comment.
git submodule update
If ble.sh works inside the store or the sandbox this will not work. If we fetch the submodules anyway we don't need git and should rather remove that line of code.
There was a problem hiding this comment.
Git is a dependency of make here. It's not a runtime-dependency.
There was a problem hiding this comment.
@SuperSandro2000 Let me correct myself. I have written above that
I guess we don't need it as far as we specify
fetchSubmodules = true?
but I was forgetting that the build-time dependency on git is not only for fetching submodules. We also need git to determine the embedded version string of ble.sh. The command git is called by this line and this line at the build time.
|
Resolved all the discussion above🎉 |
Follow more standard approach
|
Added one commit from discussion at akinomyoga/ble.sh#212 (reply in thread). |
|
I don't know what I should do to fix the EditorConfig error... Can I just ignore the failing test? nixpkgs/pkgs/shells/bash/ble.sh/default.nix Lines 39 to 53 in 8fd2f23 |
I am using it right now but I just cloned the repository.
I would recommend to not package the latest stable but unstable. We also don't want to keep compatibility code for a version thats two years old. Also upstream would likely not take any bug reports for that old version and I already reported a handful of bugs and I am barely using its full potential.
Thats a very default file which should only save you a few minutes at best.
It does not. It also works with spaces.
nope |
|
great to see work done on ble.sh on nix! |
pkgs/shells/bash/ble.sh/default.nix
Outdated
| postInstall = '' | ||
| mkdir -p "$out/bin" | ||
| cat <<SCRIPT >"$out/bin/ble.sh-share" | ||
| #!${runtimeShell} | ||
| # Run this script to find the ble.sh shared folder | ||
| # where all the shell scripts are living. | ||
| echo "$out/share/ble.sh" | ||
| SCRIPT |
There was a problem hiding this comment.
I don't think we need this at all. We can just source the files inside of programs.bash.interactiveShellInit where we can refer to $pkgs.ble-sh.
There was a problem hiding this comment.
Not all the users are using configuration.nix. And this way is described on the nixpkgs docs, which is why I assumed it is the current standard way of packaging programs like ble.sh.
There was a problem hiding this comment.
Where is that described in the docs?
If you are not on NixOS you install this with either home-manager or nix-env and would source it from the symlink in your home directory.
There was a problem hiding this comment.
Are you sure? I made a quick test for this, and the spaces does not work as intended (EOS at the end caused the error). You can just throw away the readability of the bash script by deleting the indent and use |
|
I don't think latest Do you think it's a good idea to distribute the "nightly" version, @akinomyoga? Do you have a plan to update the nightly tag? |
|
What!? Ofborg reports no error! On my environment (darwin x86-64) the check phase is broken😭 |
|
I have never run the check on macOS. Could you share the error messages for the |
This is a result when I execute `make check` on bash prompt:And this is a log created on building a derivation of this PR (quite long):Executed # Local environment which run `make check`. I just noticed that it is too old.
$ bash --version
GNU bash, version 3.2.57(1)-release (x86_64-apple-darwin21)
Copyright (C) 2007 Free Software Foundation, Inc.
# Getting into nix environment
$ nix develop local#blesh
$ runHook preCheck
$ bash --version
GNU bash, version 5.1.16(1)-release (x86_64-apple-darwin17.7.0)
Copyright (C) 2020 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>
This is free software; you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.EDIT: Sorry, I remembered that I'm using a bash which is outside of PATH as a login shell. Oh I just noticed that the bash version is already printed in log😆 |
|
@aiotter Thanks for the log of But some of the problems found in building the derivation is not yet solved. They seem to be related to the available locales in the building environment of nix. In particular, A UTF-8 locale needs to be set for |
|
It's weird that the shell invoked by |
Not that frequently to be honest. Sometimes it does not receive a commit for 3 weeks which is totally fine.
We don't need to decide that. Whenever something more than a documentation change happens we just update it or when we feel like it. Packaging a two year old version and then requiring everyone to maintain their own overlay is just much more work for everyone.
Not really. |
pkgs/shells/bash/blesh/default.nix
Outdated
| mkdir -p "$out/bin" | ||
| cat <<SCRIPT >"$out/bin/blesh-share" | ||
| #!${runtimeShell} | ||
| # Run this script to find the ble.sh shared folder | ||
| # where all the shell scripts are living. | ||
| echo "$out/share/ble.sh" | ||
| SCRIPT | ||
| chmod +x "$out/bin/blesh-share" |
There was a problem hiding this comment.
This can be easily replaced with source ~/.nix-profile/share/ble.sh.
There was a problem hiding this comment.
Is it guaranteed to be there even when it is installed by configure.nix?
Then it's okay to remove this part and we can update the docs. Docs say that there is no way to decide the path of share directory and describes a solution of PACKAGE-share.
There was a problem hiding this comment.
There was a problem hiding this comment.
Seeing that section for the first time. Maybe we can do this but I wouldn't have done it.
|
@akinomyoga Thanks for your fix! I have updated the locale to The same check is errored out both on usual environment of macOS and while building nix derivation. This build is on akinomyoga/ble.sh@0b95d5d, and I made it sure that the change introduced akinomyoga/ble.sh@fa955c1 is actually in ble.pp. |
|
@ofborg build blesh |
|
@SuperSandro2000 Can I ask one thing about commit message? You renamed this PR from |
|
I would have squash merged everything anyway. |
|
@ofborg build blesh |
There was a problem hiding this comment.
/private/tmp/nix-build-ble.sh.drv-0/source/out/lib/test-main.sh:54: ble/util/readlink f.txt [[ $ret != /* ]] && ret=${PWD%/}/$ret --- 39858.ret.expect 2022-07-24 15:32:44.000000000 +0000 +++ 39858.ret.result 2022-07-24 15:32:44.000000000 +0000 @@ -1 +1 @@ -/tmp/blesh/301/39761.test/39858.d/ab/cd/ef/file.txt +/private/tmp/blesh/301/39761.test/39858.d/ab/cd/ef/file.txt 94.7% [section] ble/main: 18/19 (1 fail, 0 crash, 0 skip)
@aiotter I have added a fix for the remaining test failure (#181963 (comment)) in ble.sh commit a22e145 @ akinomyoga/ble.sh. Could you check if the make check problem is finally solved in macOS?
I have also tested the blesh package in nixpkgs, but I have noticed a few points. I have added comments in the code in this PR (although you might want to open a new PR). Could you also take a look at them?
As discussed in #181963 (comment), I have added a stub on the installation instruction in ble.sh Wiki. Could you edit the page?
Another naive question is where the users are supposed to report an issue of a specific package in nixpkgs. Is there a standard place to report them? Or do we need to contact the package maintainer by directly sending emails or by means that each maintainer specifies? Or can we just open an issue in the upstream nixpkgs (this repository)?
| checkInputs = [ bashInteractive glibcLocales ]; | ||
| preCheck = "export LC_ALL=en_US.UTF-8"; | ||
|
|
||
| installFlags = [ "INSDIR=$(out)/share" ]; |
There was a problem hiding this comment.
| installFlags = [ "INSDIR=$(out)/share" ]; | |
| installFlags = [ "PREFIX=$(out)" ]; |
| _ble_base_package_type=nix | ||
|
|
||
| function ble/base/package:nix/update { | ||
| echo "Ble.sh is installed by Nix. You can update it there." >/dev/stderr |
There was a problem hiding this comment.
I prefer to use &2 over /dev/stderr. /dev/stderr is not POSIX.
| echo "Ble.sh is installed by Nix. You can update it there." >/dev/stderr | |
| echo "Ble.sh is installed by Nix. You can update it there." >&2 |
| #!${runtimeShell} | ||
| # Run this script to find the ble.sh shared folder | ||
| # where all the shell scripts are living. | ||
| echo "$out/share/ble.sh" |
There was a problem hiding this comment.
blesh-share should return the data directory name but not the script file. Also, the install target is strange. Currently, the related script files are somehow installed in $(out)/share, but they should be installed in $(out)/share/blesh.
| echo "$out/share/ble.sh" | |
| echo "$out/share/blesh" |
Yes or write on matrix or if you have a fix just open a PR. |
|
@akinomyoga Thank you for your patch and review. I have confirmed that p.s. I'll fix the wiki after fixing |
|
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: |
Description of changes
Adds
bre.shble.sh.Use this package by executing
source ble.shon bash.Users can modify their
~/.bashrcto have it loaded on login:See https://github.com/akinomyoga/ble.sh#13-set-up-bashrc for more detail.
Things done
sandbox = trueset innix.conf? (See Nix manual)nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/)nixos/doc/manual/md-to-db.shto update generated release notes