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

encore: init at version 1.39.0 #328519

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

luisnquin
Copy link
Member

@luisnquin luisnquin commented Jul 19, 2024

Description of changes

Things done

  • 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/)
  • 24.11 Release Notes (or backporting 23.11 and 24.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.

@luisnquin
Copy link
Member Author

Screenshots

image

image

Copy link
Member

@afh afh left a comment

Choose a reason for hiding this comment

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

Thanks for this, @luisnquin 🙂
Please find below a few questions and suggestions for improvement.

If you wanted to go above and beyond, please consider formatting the files using nixfmt.

Comment on lines 13 to 15
name = "${pname}-source";
owner = "encoredev";
repo = pname;
Copy link
Member

Choose a reason for hiding this comment

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

Please avoid superfluous use of pname (for details see #277994)

Suggested change
name = "${pname}-source";
owner = "encoredev";
repo = pname;
name = "encore-source";
owner = "encoredev";
repo = encore;

mkdir -p $out/share/runtimes
cp -r $src/runtimes/* $out/share/runtimes

ln -s ${goEncore}/bin/* $out/bin
Copy link
Member

Choose a reason for hiding this comment

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

Can you check whether it is possible to use lib.getBin here, please?

Suggested change
ln -s ${goEncore}/bin/* $out/bin
ln -s ${lib.getBin goEncore}/* $out/bin

Copy link
Member Author

Choose a reason for hiding this comment

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

Is not possible like that.

image

image

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for trying this out 👍 Not sure I understand why it doesn't work, but I'll leave that for another day 🙂

proxyVendor = true;

meta = with lib; {
description = "Encore is the Backend Development Platform purpose-built to help you create event-driven and distributed systems.";
Copy link
Member

Choose a reason for hiding this comment

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

The following suggestions follows recommendations for meta.description as outlined in pkgs/README.md

Suggested change
description = "Encore is the Backend Development Platform purpose-built to help you create event-driven and distributed systems.";
description = "Backend Development Platform to create event-driven and distributed systems";

owner = "encoredev";
repo = "go";
rev = "encore-go${version}";
sha256 = "sha256-SSa3CoUS6JMfs6T1PMb3NK8UeJnpLlTIn46/3oCTnL8=";
Copy link
Member

Choose a reason for hiding this comment

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

Use of hash is preferred over sha256 as it allows future flexibility should the hash algorithm be changed, e.g. to use sha512 instead of sha256:

Suggested change
sha256 = "sha256-SSa3CoUS6JMfs6T1PMb3NK8UeJnpLlTIn46/3oCTnL8=";
hash = "sha256-SSa3CoUS6JMfs6T1PMb3NK8UeJnpLlTIn46/3oCTnL8=";

, go
, ...
}:
stdenv.mkDerivation rec {
Copy link
Member

Choose a reason for hiding this comment

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

Use of finalAttrs is preferred over rec where possible. Remember to close the newly opened parenthesis at the end of the file:

Suggested change
stdenv.mkDerivation rec {
stdenv.mkDerivation (finalAttrs: {

description = "Encore's rolling fork of Go with added automatic tracing & instrumentation";
homepage = "https://encore.dev";
maintainers = with maintainers; [ luisnquin ];
platforms = platforms.linux;
Copy link
Member

Choose a reason for hiding this comment

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

❓ Is this really linux only?

Copy link
Member Author

Choose a reason for hiding this comment

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

@afh I have only tested this package on Linux.

Copy link
Member

Choose a reason for hiding this comment

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

According to https://encore.dev is should be possible to run on darwin, @luisnquin. Happy to help test on darwin if helpful…

Copy link
Member Author

Choose a reason for hiding this comment

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

Of course, I'm currently delivering this package in this flake first.

Just clone it, run nix build and execute the following commands:

cd "$(mktemp -d --suffix="-temp-env")"
/path/to/.../result/bin/encore app create --example=url-shortener # specify project name
cd <project-name>
/path/to/.../result/bin/encore run

You will need to have docker available in your system.

Copy link
Member

Choose a reason for hiding this comment

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

/usr/bin/env NIXPKGS_ALLOW_UNSUPPORTED_SYSTEM=1 nix build --impure github:luisnquin/nixpkgs-extra currently fails with:

trying https://github.com/encoredev/go/archive/encore-go1.22.tar.gz
 …
curl: (22) The requested URL returned error: 404

What am I doing wrong?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure about it, it works in my system (i have cleaned the store)

image

Copy link
Member

Choose a reason for hiding this comment

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

I think I have an idea what is going wrong, please allow for a few moments to verify.

'';

meta = with lib; {
description = "Encore's rolling fork of Go with added automatic tracing & instrumentation";
Copy link
Member

Choose a reason for hiding this comment

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

Ideally meta.description does not mention the package name, yet here it seems useful as this sub-package is tied to encore. Feel free to disregard this suggestions if it provides too little value.

Suggested change
description = "Encore's rolling fork of Go with added automatic tracing & instrumentation";
description = "Rolling fork of Go with added automatic tracing and instrumentation for use with the encore package";

Comment on lines 68 to 69
mkdir -p /tmp/.gobuild-cache
GOCACHE=/tmp/.gobuild-cache go run . \
Copy link
Member

@afh afh Jul 19, 2024

Choose a reason for hiding this comment

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

❓ Would this benefit from using $TMPPATH$TMPDIR rather than /tmp directly?

Suggested change
mkdir -p /tmp/.gobuild-cache
GOCACHE=/tmp/.gobuild-cache go run . \
mkdir -p $TMPDIR/.gobuild-cache
GOCACHE=$TMPDIR/.gobuild-cache go run . \

Copy link
Member Author

Choose a reason for hiding this comment

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

That variable appears to be undefined on my system.

image

Copy link
Member

Choose a reason for hiding this comment

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

Apologies, I meant $TMPDIR.

Copy link
Member Author

Choose a reason for hiding this comment

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

That worked

cp -r ${goSrc} go
chmod -R u+rw go

cd go
Copy link
Member

Choose a reason for hiding this comment

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

Would using pushd/popd convey the intent more clearly?

Suggested change
cd go
pushd go


cp -p -P -v -R ../overlay/* ./

cd ..
Copy link
Member

Choose a reason for hiding this comment

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

v.s.

Suggested change
cd ..
popd

@ofborg ofborg bot added 8.has: package (new) This PR adds a new package 11.by: package-maintainer This PR was created by the maintainer of the package it changes 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild 10.rebuild-linux: 1-10 10.rebuild-linux: 1 labels Jul 19, 2024
luisnquin added a commit to 0xc000022070/nixpkgs-extra that referenced this pull request Jul 19, 2024
@luisnquin
Copy link
Member Author

@afh I just applied most of the suggested changes with the nixfmt format :)

Copy link
Member

@afh afh left a comment

Choose a reason for hiding this comment

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

Thanks for addressing the review comments and accepting the changes much appreciated 🙏

Please find below more changes that were needed in order to build this on aarch64-darwin.
With these changes does this PR still build for you on *-linux?

{ fetchFromGitHub, stdenv, lib, go, ... }:
stdenv.mkDerivation (finalAttrs: {
pname = "go-encore";
version = "1.22";
Copy link
Member

Choose a reason for hiding this comment

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

Version is missing the patch number as published on https://github.com/encoredev/go/releases/tag/encore-go1.22.0

Suggested change
version = "1.22";
version = "1.22.0";

goSrc = fetchFromGitHub {
owner = "golang";
repo = "go";
rev = "release-branch.go${finalAttrs.version}";
Copy link
Member

Choose a reason for hiding this comment

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

The golang repo branches do not use the patch-number, so let's strip it (see https://github.com/golang/go/tree/release-branch.go1.22):

Suggested change
rev = "release-branch.go${finalAttrs.version}";
rev = "release-branch.go${lib.versions.majorMinor finalAttrs.version}";

@afh
Copy link
Member

afh commented Jul 19, 2024

ℹ️ Maybe I'm missing something (or nixfmt's format) has changed, but the files in this PR do not appear to be properly formatted using nixfmt…
More specifically I'd expect the input parameters on the first line to be split on one-per line.

@luisnquin
Copy link
Member Author

@afh here are details about my nixfmt

image

@afh
Copy link
Member

afh commented Jul 19, 2024

When I run nixfmt pkgs/by-name/en/encore/ changes are made to files in pkgs/by-name/en/encore/, so it appears that the changes nixfmt makes are not committed to this PRs branch.
Can you double check, please, @luisnquin?

@afh
Copy link
Member

afh commented Jul 19, 2024

Result of nixpkgs-review pr 328519 run on aarch64-darwin 1

1 package built:
  • encore

😄🎉

Comment on lines 69 to 70
ln -s $out/share/go/bin/go $out/bin
mv $out/bin/go $out/bin/go-encore
Copy link
Member

Choose a reason for hiding this comment

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

❓ Maybe I'm missing something but couldn't this be shorted to the following?

Suggested change
ln -s $out/share/go/bin/go $out/bin
mv $out/bin/go $out/bin/go-encore
ln -s $out/share/go/bin/go $out/bin/go-encore

@luisnquin
Copy link
Member Author

luisnquin commented Jul 19, 2024

@afh I'm applying those changes to the correct repository, you can see a recording in the following link https://drive.google.com/file/d/1LFn5snxJoS4tWv4acXeY4aPMc1FBmgTC/view

gss = git status -s

@afh
Copy link
Member

afh commented Jul 19, 2024

Could it be that the latest changes containing the nixfmt changes in your workspace have not yet been pushed to this PRs branch?

@afh
Copy link
Member

afh commented Jul 19, 2024

Re nixfmt, I was still using (and am still used to) the format from nixfmt 0.5.0 and wasn't aware 0.6.0 with changes was released. Thanks for your patience on this, @luisnquin 🙂

Copy link
Member

@afh afh left a comment

Choose a reason for hiding this comment

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

This is shaping up nicely, @luisnquin, I think one last thing… 😬

hash = "sha256-amASGhvBcW90dylwFRC2Uj4kOAOKCgWmFKhLnA9dOgg=";
};
in ''
# delete submodule and replace with the go source
Copy link
Member

Choose a reason for hiding this comment

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

I think it's good practice to ensure the pre- and post- hooks are run for each phase:

Suggested change
# delete submodule and replace with the go source
runHook prePatch
# delete submodule and replace with the go source


cp -p -P -v -R ../overlay/* ./

popd
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
popd
popd
runHook postPatch

}.${platform.parsed.cpu.name} or (throw
"Unsupported system: ${platform.parsed.cpu.name}");
in ''
mkdir -p $TMPDIR/.gobuild-cache
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
mkdir -p $TMPDIR/.gobuild-cache
runHook preInstall
mkdir -p $TMPDIR/.gobuild-cache

cp -r dist/${stdenv.targetPlatform.parsed.kernel.name}_${
goarch stdenv.targetPlatform
}/encore-go/* $out/share/go
ln -s $out/share/go/bin/go $out/bin/go-encore
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
ln -s $out/share/go/bin/go $out/bin/go-encore
ln -s $out/share/go/bin/go $out/bin/go-encore
runHook postInstall

@fmway
Copy link

fmway commented Jul 19, 2024

yo guys sorry. I just built it. But there's one tool missing (tsparser-encore).
image
it's here. https://github.com/encoredev/encore/blob/main/tsparser/Cargo.toml

@afh
Copy link
Member

afh commented Jul 19, 2024

Result of nixpkgs-review pr 328519 run on aarch64-darwin 1

1 package built:
  • encore

doCheck = true;

subPackages =
[ "cli/cmd/encore" "cli/cmd/git-remote-encore" "cli/cmd/tsbundler-encore" ];
Copy link
Member

Choose a reason for hiding this comment

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

The following suggestion just 💅 feel free to disregard 🙂

Suggested change
[ "cli/cmd/encore" "cli/cmd/git-remote-encore" "cli/cmd/tsbundler-encore" ];
map (x: "cli/cmd/" + x) [ "encore" "git-remote-encore" "tsbundler-encore" ];

Copy link

Choose a reason for hiding this comment

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

very nixification :v

Copy link
Member Author

@luisnquin luisnquin Jul 19, 2024

Choose a reason for hiding this comment

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

@afh I prefer to leave it like this for now

Anyway, thanks for your suggestion

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for considering it, @luisnquin.

Do I understand fmway's comment correctly that more work on this PR is necessary in order to install tsparser-encore or is this ready for final review?

Copy link
Member Author

Choose a reason for hiding this comment

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

@afh I'm currently trying to build tsparser-encore too but it seems that it may take me some time

Here's my current progress

image

I have a problem trying to get the correct hashes

image

nix keeps complaining about it

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, I think that it is no longer necessary when a lock file is passed, still, this seems somewhat tricky to maintain to me

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for this, @luisnquin.

At first glance I have the following comments:

  • Rename the encoreSrc parameter just src
  • Use inherit version src; instead of assigning version and encoreSrc
  • A comment on the Cargo.lock operations (rm and cp) as well as on each substituteInPlace would be very helpful
  • Use "${lib.getExe protobuf}" instead of "${protobuf}/bin/protoc"

What will be tricky to maintain from your point of view?

Copy link
Member Author

Choose a reason for hiding this comment

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

At the moment, the way how the Cargo.lock file is being configured and the "subtitute in place" statements

Copy link
Member

Choose a reason for hiding this comment

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

Can you provide more context as to why they are necessary? Maybe these should be raised with upstream, unless they are specific to nixpkgs…

Copy link
Member Author

Choose a reason for hiding this comment

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

Because there's a lifetime error in the Rust code, I'm not sure if that only happens on Nix since I wasn't able to build it with my normal shell due to a lack of time

image

@luisnquin
Copy link
Member Author

@afh by my side, the tsparser requires to update their tracing crates.

I've spent many hours on this, and if you agree, we can deliver the package in the current state until encoredev/encore#1295 is resolved. At least for now, there’s no problem using it with Go.

image

image

@afh
Copy link
Member

afh commented Jul 20, 2024

That sounds very reasonable to me, @luisnquin, thanks for putting in the effort, much appreciated 🙏

Copy link
Member

@afh afh left a comment

Choose a reason for hiding this comment

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

Thanks for polishing this up, well done! 👏

Find below an optional minor suggestion and feel free to disregard. Looking forward to seeing this get merged 🙂

Comment on lines 65 to 70
mkdir -p $TMPDIR/.gobuild-cache
GOCACHE=$TMPDIR/.gobuild-cache go run . \
--goos "${stdenv.targetPlatform.parsed.kernel.name}" \
--goarch "${goarch stdenv.targetPlatform}"

mkdir -p $out/{bin,share/go}
Copy link
Member

Choose a reason for hiding this comment

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

Given that mkdir allows to create multiple directories simultaneously this could be shortened to, feel free to disregard if it hinders legibility.

Suggested change
mkdir -p $TMPDIR/.gobuild-cache
GOCACHE=$TMPDIR/.gobuild-cache go run . \
--goos "${stdenv.targetPlatform.parsed.kernel.name}" \
--goarch "${goarch stdenv.targetPlatform}"
mkdir -p $out/{bin,share/go}
mkdir -p $TMPDIR/.gobuild-cache $out/{bin,share/go}
GOCACHE=$TMPDIR/.gobuild-cache go run . \
--goos "${stdenv.targetPlatform.parsed.kernel.name}" \
--goarch "${goarch stdenv.targetPlatform}"

@afh
Copy link
Member

afh commented Jul 20, 2024

Result of nixpkgs-review pr 328519 run on aarch64-darwin 1

1 package built:
  • encore

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/prs-already-reviewed/2617/1843

@luisnquin luisnquin force-pushed the package/encore branch 2 times, most recently from fd1187a to 57e794d Compare July 22, 2024 20:25
Comment on lines 31 to 35
tsParser = callPackage ./tsparser.nix {
inherit src version;

runtimesPath = "${placeholder "out"}/share/runtimes";
};
Copy link
Member

Choose a reason for hiding this comment

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

Nice!! 👍

Comment on lines 20 to 22
version = version;

inherit src;
Copy link
Member

Choose a reason for hiding this comment

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

This can be shortened to:

Suggested change
version = version;
inherit src;
inherit version src;

Copy link
Member Author

Choose a reason for hiding this comment

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

Ups

@luisnquin
Copy link
Member Author

luisnquin commented Jul 22, 2024

@afh I just pushed new changes with the tsparser included

However @fmway you can find those changes in this flake too

@luisnquin luisnquin force-pushed the package/encore branch 2 times, most recently from 80430e3 to d2022f8 Compare July 22, 2024 21:07
@afh
Copy link
Member

afh commented Jul 22, 2024

The latest changes on your flake worked well for me, @luisnquin, well done! Is there anything you'd like to address or are the changes in this PR ready for review from your point of view?

@luisnquin
Copy link
Member Author

@afh yes, changes can be reviewed

@afh
Copy link
Member

afh commented Jul 22, 2024

Result of nixpkgs-review pr 328519 run on aarch64-darwin 1

1 package built:
  • encore

Copy link
Member

@afh afh left a comment

Choose a reason for hiding this comment

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

Thanks for putting in all the hard work, @luisnquin, much appreciated! From my (likely limited 😅) nixpkgs perspective things look good to me. Well done!

substituteInPlace Cargo.toml \
--replace 'members = ["runtimes/core", "runtimes/js", "tsparser"]' 'members = ["tsparser"]' \

substituteInPlace tsparser/src/bin/tsparser-encore.rs \
Copy link

Choose a reason for hiding this comment

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

it is strange that you need to do these substitutions 🤔 what rust version is this built with?

Copy link

Choose a reason for hiding this comment

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

(we always build with latest stable, so currently 1.79.0)

Copy link
Member Author

@luisnquin luisnquin Jul 24, 2024

Choose a reason for hiding this comment

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

In the unstable channel we are at 1.78.0

image

Can you test that version by switching it in your local toolchain?

Copy link

Choose a reason for hiding this comment

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

I tried downgrading, and I got the same issue on 1.78.0. Would it be an option to use something like fenix? they have a more up to date rust version.

Copy link
Member Author

Choose a reason for hiding this comment

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

Is it possible to use a flake in nixpkgs? I will research more about it.

Another option I was thinking about was to use the binaries provided by the releases that https://encore.dev/install.sh uses, at least until we can properly package with the source code.

In the current state of the package, we are not even setting the version in the binary and I'm having some problems when trying to compile a project (tsparser doesn't have a dist directory in node_modules), but if I use the binaries from the release URL I don't have a problem trying to compile a TypeScript project.


# to avoid to compile "broken" members
rm -rf runtimes
substituteInPlace Cargo.toml \
Copy link

Choose a reason for hiding this comment

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

why do you need this substitute? is that because of the stray lock file? from what I understood it should be enough to just set buildAndTestSubdir as you do on line 30 to build a sub-package

Copy link
Member Author

Choose a reason for hiding this comment

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

If I remove those subtitute lines I have the following error:

image

I'm not a Rust expert, so if you have any ideas why that happens, I'm open.

Copy link

Choose a reason for hiding this comment

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

Did you also remove the "rm -rf runtimes"? I'm wondering if it just tries to load all the Cargo.toml files from the workspace maybe

Copy link
Member Author

@luisnquin luisnquin Jul 26, 2024

Choose a reason for hiding this comment

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

It was trying to compile every workspace member... maybe I'm doing something wrong but that was a progress while trying to package this project.

@luisnquin
Copy link
Member Author

At least fetching from the releases we're able have binaries properly signed. This way will continously work until a perfect build is done.

If you're interested in my advance, please contact me at my email adress shared on my profile.

@luisnquin luisnquin marked this pull request as ready for review August 24, 2024 18:52
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.

6 participants