Closes #412559

@QuiNzX shared this code that uses buildPythonPackage instead, doesn't need us to vendor the lockfile (because PyPI has it), and would be simpler to maintain:

{
  lib,
  python3,
  fetchPypi,
  rustPlatform,
  maturin,
}:
python3.pkgs.buildPythonPackage rec {
  pname = "pyrefly";
  version = "0.17.1";
  format = "pyproject";

  src = fetchPypi {
    inherit pname version;
    hash = "sha256-w4ivRtmApXiXQT95GI4vvYBop7yxdbbkpW+YTyFtgXM=";
  };

  cargoDeps = rustPlatform.fetchCargoVendor {
    inherit src;
    hash = "sha256-Op5ueVkzZTiJ1zeBGVi8oeLcfSzXMYfk5zEg4OGyA5g=";
  };

  build-system = [ maturin ];

  nativeBuildInputs = with rustPlatform; [
    cargoSetupHook
    maturinBuildHook
  ];

  # requires unstable rust features
  env.RUSTC_BOOTSTRAP = 1;

  meta = with lib; {
    description = "A fast type checker and IDE for Python";
    homepage = "https://github.com/facebook/pyrefly";
    license = lib.licenses.mit;
    mainProgram = "pyrefly";
    platforms = lib.platforms.linux ++ lib.platforms.darwin;
    maintainers = with lib.maintainers; [ cybardev ];
  };
}

The caveat being, with buildRustPackage we only produced a single binary (result/bin/pyrefly) but with this we also produce a result/lib which contains a bunch of Python stuff. Not sure what to prioritize here - maintainability, or package size for user?

Interesting. I didn't know that the PyPI version has the lockfile - that is a very peculiar choice IMO. (We normally prefer packaging software directly from source, but I guess we can make an exception for this one.) I think the Python stuff is mainly for using Pyrefly as a wheel (Python library)? I'm not sure how, but it does appear to be something upstream supports. In that case, then I think it's fair to build this as a Python + Maturin package instead.

Ok, sorry, I saw your message after the rebase push... I will do the Python version then.

Also if we were to support the case of using Pyrefly as a library, then we should move the main derivation to pkgs/development/python-modules and leave the one inside pkgs/by-name as a sort-of alias, like this:

{
  python3Packages
}:
python3Packages.toPythonApplication python3Packages.pyrefly

See the Nixpkgs manual for more detailed reasoning as to why we want to do this

I can check with upstream but from what I understand it cannot be used like a Python library. That is, it is pip installable to get the command-line tool but not import pyreflyable in Python code. In that case I assume it's fine to leave it in by-name?

Oh yeah, then leave it in by-name and instead use buildPythonApplication and not buildPythonPackage like the snippet you have

I think I've ironed out the issues. Let me know if there's anything to change. Thanks for the review and help. 🙂

@pluiedev One more thing. @QuiNzX would like to be added as a maintainer. Do I add a new commit to add to maintainers-list.nix, then force-push a rebase of the package init commit with the new maintainer? Not sure how to approach this best...

Add another commit adding them as a maintainer, and then rebase the main commit on top of that I think

I think I got it right? Let me know if this is ok or changes are needed. Thanks.

(apologies, I have not used Git to this extent before 😓)

Thanks for the reviews, help, and guidance. 😊

I am so sorry. I must've been mistaken. I was wrong about pyproject = true;:
https://github.com/NixOS/nixpkgs/blob/master/doc/languages-frameworks/python.section.md#buildpythonpackage-parameters-buildpythonpackage-parameters

• pyproject: Whether the pyproject format should be used. As all other formats
  are deprecated, you are recommended to set this to true. When you do so,
  pypaBuildHook will be used, and you can add the required build dependencies
  from build-system.requires to build-system. Note that the pyproject
  format falls back to using setuptools, so you can use pyproject = true
  even if the package only has a setup.py. When set to false, you can
  use the existing hooks or provide your own logic to build the
  package. This can be useful for packages that don't support the pyproject
  format. When unset, the legacy setuptools hooks are used for backwards
  compatibility.

It will definitely work when unset, so it doesn't entirely matter. But apparently the best practice isn't to only set it when needed, it should almost always be set, but it just isn't a default yet. They did set a pyproject.toml upstream, so I guess it should be set after all.

Sorry for the confusion and the extra work, was just trying to do a thorough review, but I kinda achieved the opposite of what I wanted. I am new here, so I still make a bunch of mistakes.

That being said, I see no reason why this could not get merged in its current state. The behaviour is the same whether it's set or not, because it's using maturin anyways, so it doesn't reach the setuptools fallback codepath.

@dtomvan No probs. Reverted in de8144a . Thanks for the thorough check. I appreciate it. 😊

nixpkgs-review result

Generated using nixpkgs-review.

Command: nixpkgs-review pr 412863
Commit: de8144a7e356e0adfcae232d3a3bd25522e75ad1

x86_64-linux

aarch64-linux

After locally verifying it works, I added nix-update-script in b92f1fd