-
-
Notifications
You must be signed in to change notification settings - Fork 13.7k
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
instant-scala: init at 0.1.0 #337451
base: master
Are you sure you want to change the base?
instant-scala: init at 0.1.0 #337451
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rename your PR title to: instant-scale: init at 0.1.0
.
Move the package to pkgs/by-name. We are currently moving all package in pkgs/top-level/all-packages.nix to pkgs/by-name
sha256 = "jqSvKTL8NzqjwqDj/+55YWecx2bnzuArP8RdfH5q/1U="; | ||
}; | ||
|
||
propagateBuildInputs = [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why use propagateBuildInputs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The idea is that any downsteram packages (if any) that uses instant-scala will require bash & scala-cli to run anyway.
(If I'm understanding it right.. Basing my interpretation off https://gist.github.com/CMCDragonkai/45359ee894bc0c7f90d562c4841117b5)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have changed it to buildInputs for now but keen to learn which way is correct based on what this script (instant-scala) is doing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, first of all, it should be propagatedBuildInputs
, so I don't believe this is doing anything but defining a list that is not used?
propagatedBuildInputs is only for buildInputs that have to be "on the PATH" during a build of something that depends on your derivation. Instead you should patch the script, with patchShebangs and/or manually so that that is not necessary.
|
||
propagateBuildInputs = [ | ||
pkgs.bash | ||
pkgs.scala-cli |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will this work?
Thanks a lot for the thorough review @Bot-wxt1221! I'll check and get them fixed as soon as I got time. |
52c2d14
to
b363720
Compare
src = fetchFromGitHub { | ||
owner = "jatcwang"; | ||
repo = "instant-scala"; | ||
rev = "/refs/tags/v${finalAttrs.version}"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rev = "/refs/tags/v${finalAttrs.version}"; | |
rev = "refs/tags/v${finalAttrs.version}"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not fixed actually
}; | ||
|
||
buildInputs = with pkgs; [ | ||
bash |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean that bash is included by default.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see I didn't know that. Removed
lib, | ||
stdenv, | ||
fetchFromGitHub, | ||
pkgs, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do not include the hole pkgs. just include the package you used like scale-cli.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!
mkdir -p $out/bin | ||
cp ${finalAttrs.pname} $out/bin/${finalAttrs.pname} | ||
chmod +x $out/bin/${finalAttrs.pname} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mkdir -p $out/bin | |
cp ${finalAttrs.pname} $out/bin/${finalAttrs.pname} | |
chmod +x $out/bin/${finalAttrs.pname} | |
install -Dm755 instant-scala $out/bin/instant-scala |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Avoid using finalAttrs.pname as program name because people may override the pname.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That makes sense. What should I use instead? Sorry I'm not familiar with latest best practices.
I found #310373 but it doesn't seem obvious to me what I should use.
The official doc https://nixos.org/manual/nixpkgs/stable/ is using rec
🫨
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just use instant-scala
. Don't use pname as the name of binary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure I can do that. What about version
?
(I'm trying to learn what the best practice is - if I do want to reuse variables but still allow downstream to override)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's OK. The reason why don't use pname is for override. If we override pname. Build will fail.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated
homepage = "https://github.com/jatcwang/${finalAttrs.pname}"; | ||
license = lib.licenses.asl20; | ||
maintainers = [ lib.maintainer.jatcwang ]; | ||
mainProgram = finalAttrs.pname; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will this really work?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know will darwin work for this installPhase.
src = fetchFromGitHub { | ||
owner = "jatcwang"; | ||
repo = "instant-scala"; | ||
rev = "/refs/tags/v${finalAttrs.version}"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not fixed actually
mkdir -p $out/bin | ||
cp instant-scala $out/bin/instant-scala | ||
chmod +x $out/bin/instant-scala |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use install
instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use
install
instead.
Sorry, my metallic friend. You'd use install
if there was an installer script or makefile, but the repo has a plain bash script. The install phase, as written, looks correct.
From the manual:
"The default installPhase creates the directory $out
and calls make install
"
Since there's no makefile, creating $out
and copying appropriately is the right answer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bot-wxt1221 is right, use install -Dm755 instant-scala -t $out/bin
instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Bot-wxt1221 my apologies. TIL.
Please fix the title of this PR -- it should be |
jatcwang = { | ||
email = "[email protected]"; | ||
github = "jatcwang"; | ||
githubId = 4957161; | ||
name = "Jacob Wang"; | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is normally done as its own commit titledmaintainers: add jatcwang
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
description = "Write Scala scripts that starts instantly using scala-cli and GraalVM"; | ||
homepage = "https://github.com/jatcwang/instant-scala"; | ||
license = lib.licenses.asl20; | ||
maintainers = [ lib.maintainer.jatcwang ]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maintainers = [ lib.maintainer.jatcwang ]; | |
maintainers = with lib.maintainers; [ jatcwang ]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Causes a build failure that keeps me from testing the Darwin build. Sorry.
license = lib.licenses.asl20; | ||
maintainers = [ lib.maintainer.jatcwang ]; | ||
mainProgram = "instant-scala"; | ||
platforms = lib.platforms.unix; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
platforms = lib.platforms.unix; |
Not needed, should be fine on all platforms that run Scala.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed
Rename your second commit message to |
|
||
meta = { | ||
description = "Write Scala scripts that starts instantly using scala-cli and GraalVM"; | ||
homepage = "https://github.com/jatcwang/instant-scala"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
miss changelog here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed sorry
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
meta = { | ||
description = "Write Scala scripts that starts instantly using scala-cli and GraalVM"; | ||
homepage = "https://github.com/jatcwang/instant-scala"; | ||
changelog = "https://github.com/jatcwang/instant-scala/releases"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changelog = "https://github.com/jatcwang/instant-scala/releases"; | |
changelog = "https://github.com/jatcwang/instant-scala/releases/v${finalAttrs.version}"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done thanks
meta = { | ||
description = "Write Scala scripts that starts instantly using scala-cli and GraalVM"; | ||
homepage = "https://github.com/jatcwang/instant-scala"; | ||
changelog = "https://github.com/jatcwang/instant-scala/releases/${finalAttrs.version}"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changelog = "https://github.com/jatcwang/instant-scala/releases/${finalAttrs.version}"; | |
changelog = "https://github.com/jatcwang/instant-scala/releases/v${finalAttrs.version}"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated!
Result of 1 package built:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Result of 1 package built:
|
Description of changes
Add instant-scala (https://github.com/jatcwang/instant-scala), a wrapper script around scala-cli/GraalVM which allow writing scala scripts that starts instantly.
Things done
nix.conf
? (See Nix manual)sandbox = relaxed
sandbox = true
nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD"
. Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/
)Add a 👍 reaction to pull requests you find important.