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.

Sign up for GitHub

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

Jump to bottom

cargo-lock translator attempting to build packages outside of requirment for rust package #265

Open
bezmuth opened this issue Aug 24, 2022 · 8 comments · May be fixed by #270
Open

cargo-lock translator attempting to build packages outside of requirment for rust package #265

bezmuth opened this issue Aug 24, 2022 · 8 comments · May be fixed by #270
Labels
bug Something isn't working rust

Comments

@bezmuth
Copy link
Contributor

bezmuth commented Aug 24, 2022 •
edited
Loading

Attempting to build:

{
  inputs = {
    nixpkgs.url = "github:nixos/nixpkgs/nixos-unstable";
    dream2nix.url = "github:nix-community/dream2nix";
    src.url = "github:nushell/nushell/0.67.0";
    src.flake = false;
  };

  outputs = {
    self,
    dream2nix,
    nixpkgs,
    src,
    ...
  } @ inp:
    (
      let
        pkgs = nixpkgs.legacyPackages."x86_64-linux";
      in
      dream2nix.lib.makeFlakeOutputs {
      systems = ["x86_64-linux"];
      config.projectRoot = ./.;
      source = src;
      packageOverrides."^.*".addDeps = {
        nativeBuildInputs = old: old ++ [pkgs.pkgconfig pkgs.python3];
        buildInputs = old: old ++ [pkgs.openssl pkgs.zstd pkgs.xorg.libX11];
        doCheck = false;
        buildFeatures = ["stable"];
        cargoUpdateHook = ''
          cargo add zstd-sys --features pkg-config --offline
          cargo update --package zstd-sys --offline
        '';
        shellPath = "/bin/nu";
      };

      settings = [
        {
          translator = "cargo-lock";
        }
      ];
    });
}

Results in this error:

error: attribute 'Cargo.lock' missing

       at /nix/store/3akswl509msksq037fps8mzb5m66ba7z-source/src/subsystems/rust/translators/cargo-lock/default.nix:94:18:

           93|     # Parse Cargo.lock and extract dependencies
           94|     parsedLock = projectTree.files."Cargo.lock".tomlContent;
             |                  ^
           95|     parsedDeps = parsedLock.package;
(use '--show-trace' to show detailed location information)
------------------------------------------------------------

Attaching a debugger to the build process and checking what package is being built by viewing "projectTree.files" shows a package without a cargo lock is being built.
Furthermore this package isn't part of the dependencies for nushell and instead looks to be used as a sample of some sort (https://github.com/nushell/nushell/tree/main/samples/wasm).
I believe this is something to do with the discoverer loading all directories that contain a cargo.toml, however as the cargo-lock translator only expects dirs with both a cargo.toml and a cargo.lock it fails.
@DavHau DavHau added bug Something isn't working rust labels Aug 25, 2022
@bezmuth bezmuth mentioned this issue Aug 26, 2022
Closed
@yusdacra
Copy link
Member

This behaviour makes sense to me, because what you are basically telling dream2nix to do is that it should use the cargo-lock translator for all packages, which of course fails because not all packages in the tree has a Cargo.lock. Instead of the linked PR, we should make it fallback to another translator if the one user wants is not supported. We already add supported translators to a translators attribute in project attrset, so it should just be a matter of checking if the translator the user wants is available there, and if not just select the first translator in that list.

@yusdacra yusdacra linked a pull request Aug 26, 2022 that will close this issue
Open
@bezmuth
Copy link
Contributor Author

bezmuth commented Aug 27, 2022

This behaviour makes sense to me, because what you are basically telling dream2nix to do is that it should use the cargo-lock translator for all packages, which of course fails because not all packages in the tree has a Cargo.lock. Instead of the linked PR, we should make it fallback to another translator if the one user wants is not supported. We already add supported translators to a translators attribute in project attrset, so it should just be a matter of checking if the translator the user wants is available there, and if not just select the first translator in that list.

Couldn't this cause impure builds to occur when the user attempts to do a pure build?

@yusdacra
Copy link
Member

Couldn't this cause impure builds to occur when the user attempts to do a pure build?

No, impure builds aren't a thing in dream2nix. We only have impure translation. Building shouldn't matter here since this is only about translators.

@bezmuth
Copy link
Contributor Author

bezmuth commented Aug 28, 2022

@yusdacra's solution makes more sense (especially for nix flake show, not displaying all the projects in a source tree seems like a bad idea), I was wondering if a warning should be displayed if a fallback translator has to be selected?

@yusdacra
Copy link
Member

@yusdacra's solution makes more sense (especially for nix flake show, not displaying all the projects in a source tree seems like a bad idea), I was wondering if a warning should be displayed if a fallback translator has to be selected?

I think this is a good idea, we could probably display a warning and then say something like "to disable this warning, don't apply your translator settings to these projects by setting filter = project: project.name != something in your settings". I can't remember how filter is used off the top of my head, but we could maybe add a convenience function for that or something. Or maybe just make it a configuration option like config.disableIfdWarning. Not sure which would be better.

@DavHau
Copy link
Member

DavHau commented Aug 31, 2022

I think we should not silently ignore what the users specified. This might lead to unexpected behavior elsewhere.
If we can tell for sure, that whatever the user wants will not work, we should exit and throw an error with explanation on how it could potentially be fixed.

Also I think that it is a valid use case to specify a translator which is not detected as compatible. Maybe there is a problem in the auto-detection, and the user wants to force override.

I think we should fix it in the cargo-lok translator itself. It could check if a cargo.lock exists and if not, error out recommending to use the cargo-toml translator instead.

@yusdacra
Copy link
Member

We can add an error message in cargo-lock, but we would still need to present users a way so that they can only apply the translator settings to certain projects and not all of the projects, which in this case breaks stuff. I'm not sure if filter would work here, but we need something like that and recommend it to users in the error message. And then they can add another settings that make some projects use cargo-toml, as an example.

@DavHau
Copy link
Member

DavHau commented Aug 31, 2022

In the error message we could suggest to set

{
  filter = project: project.relPath == "some/path";
  project.translator = [cargo-toml];
}

This whole project settings API was a quick hack and is bad. We should replace it. Potentially while migrating to the module system.

As an intermediary improvement, we could refactor just the settings to be its own module evaluation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug Something isn't working rust
Projects
None yet
Milestone
No milestone
3 participants
@yusdacra @bezmuth @DavHau