Skip to content
This repository has been archived by the owner on Aug 31, 2023. It is now read-only.

Accept an environment variable to specify which binary to use #4539

Merged
merged 2 commits into from
Jun 13, 2023

Conversation

framp
Copy link
Contributor

@framp framp commented May 29, 2023

Summary

This adds an env var which allow users to override the path to the rome binary.

It's a way to provide a solution for running rome under nix.
See #4516 (comment)

Test Plan

Changelog

  • The PR requires a changelog line

Documentation

  • The PR requires documentation
  • I will create a new PR to update the documentation

framp added 2 commits May 29, 2023 10:09
This is a way to temporary fix this build issue: rome#4516

I think it's a nice feature regardless, for advanced users who are experiencing issues
@netlify
Copy link

netlify bot commented May 29, 2023

Deploy Preview for docs-rometools canceled.

Built without sensitive environment variables

Name Link
🔨 Latest commit a70f684
🔍 Latest deploy log https://app.netlify.com/sites/docs-rometools/deploys/6474508742972400076361a0

@ematipico
Copy link
Contributor

This is a way to temporary fix this build issue: #4516

What would be a good fix for your issue, instead?

@framp
Copy link
Contributor Author

framp commented May 30, 2023

Debugging the issue on nixos and finding out what is causing the sigsegv.

I'd still like to have an env var specifying the binary as it can be pretty useful.

Prisma does something similar which I frequently use:
https://github.com/pimeys/nix-prisma-example/blob/main/flake.nix#L17

@jrouleau
Copy link

This is a way to temporary fix this build issue: #4516

What would be a good fix for your issue, instead?

I suspect @framp may be referring to a case where the shipped binary works on Nix (i.e. has no links to shared libraries) though I suspect this may not be practical; however, I am by no means an expert on this. The root issue here is that Nix needs a binary that is compiled for/compatible with the Nix environment, so either

A) the shipped binary is compatible with Nix, or
B) there exists a way to specify which binary to use

@framp framp mentioned this pull request May 30, 2023
1 task
Copy link
Contributor

@ematipico ematipico left a comment

Choose a reason for hiding this comment

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

This looks good to me. Thank you @framp ! I would expect a follow up PR for the documentation :)

You can open a PR against release/12.2.0

@ematipico ematipico merged commit 93b8a25 into rome:main Jun 13, 2023
7 checks passed
@ematipico
Copy link
Contributor

@framp can you try the latest nightly and see if that works as you expect?

@framp
Copy link
Contributor Author

framp commented Jun 14, 2023

Thanks!
Sure, where would you recommend adding the documentation?

I could add a few lines here:
https://github.com/rome/tools/blob/release/12.2.0/website/src/pages/guides/getting-started.mdx

@ematipico
Copy link
Contributor

Thanks!
Sure, where would you recommend adding the documentation?

I could add a few lines here:
https://github.com/rome/tools/blob/release/12.2.0/website/src/pages/guides/getting-started.mdx

Considering that you suggested that this is an advanced use-case, maybe the getting started section might not be a good fit. Maybe the CLI section? With small explanation that tells us what's it for

@Ashvith10
Copy link

@framp would you mind sharing how you would go about setting this up, perhaps by sharing an example shell.nix file? I'm new to NixOS, and usually stick to nix-shell -p <package>.

@framp
Copy link
Contributor Author

framp commented Jul 22, 2023

That would look like this @Ashvith10:

let
    pkgs = import <nixpkgs> {
    };
in
    pkgs.mkShell {
        buildInputs = [
            pkgs.nodejs_20
            pkgs.rome
        ];
        shellHook = with pkgs; ''
          export ROME_BINARY="${pkgs.rome}/bin/rome"
        '';
    }

@Ashvith10
Copy link

Ashvith10 commented Jul 22, 2023

@framp thanks for sharing the Nix file. However, I'm a little skeptical about this method, because I've faced something similar while working with Cypress (the testing framework), and the problem was that Cypress binaries were behind by a considerable version (12.16.0 when I was working vs 12.9.0 on Nix).

In my case however, Rome worked just fine and I did not get to experience the issues I've had to deal with while using Cypress.

Is is possible to over-ride the "version" variable in the Nix files, while using shell.nix?

@framp
Copy link
Contributor Author

framp commented Jul 22, 2023

You can't but you could have a local derivation which provides the latest package (by copying the derivation you linked). Or you could submit a PR on nixpkgs

You can also just patch the binary like this:
#4516 (comment)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants