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

Handle installation when not running from a full git checkout #516

Merged
merged 1 commit into from Oct 27, 2020

Conversation

newhouseb
Copy link
Contributor

@newhouseb newhouseb commented Oct 27, 2020

I'm working on setting up a Nix shell environment, which builds nmigen from a checkout that does not contain a full .git directory. In the this scenario, setuptools_scm is installed correctly, but parse/parse_git returns None because there's no active git checkout.

As a fix, I check to see if git is falsey and, if so, return a blank version string.

I've verified that this installs properly from nix via fetchFromGithub

@codecov
Copy link

codecov bot commented Oct 27, 2020

Codecov Report

Merging #516 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #516   +/-   ##
=======================================
  Coverage   81.37%   81.37%           
=======================================
  Files          49       49           
  Lines        6410     6410           
  Branches     1280     1280           
=======================================
  Hits         5216     5216           
  Misses       1005     1005           
  Partials      189      189           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e3207b7...6e585a6. Read the comment docs.

@whitequark
Copy link
Member

Is this for Nixpkgs?

@whitequark whitequark merged commit 765c15c into amaranth-lang:master Oct 27, 2020
@newhouseb
Copy link
Contributor Author

newhouseb commented Oct 27, 2020

I might build up to submitting to Nixpkgs, but am just setting up a shell for the moment (I'm a nix noob).

For future travelers, here's an example package definition (note this is against my fork, rather than the merged diff, but should work if you swap out the owner, rev and sha256 with the appropriate bits):

    nmigen = python38.pkgs.buildPythonPackage rec {
      pname = "nmigen";
      version = "0.2";

      src = pkgs.fetchFromGitHub {
        name = "nmigen";
        owner = "newhouseb";
        repo = "nmigen";
        rev = "6e585a6ad850cbc02ab730acbb01f17f08eb7152";
        sha256 = "121fa20cd5q9ijbihpcj04j2pil78h2i1gdciq2ysj2vjkg46zk8";
        fetchSubmodules = true;
      };

      doCheck = false;

      propagatedBuildInputs = [ python38.pkgs.numpy python38.pkgs.jinja2 python38.pkgs.pyvcd python38.pkgs.importlib-resources ];
      nativeBuildInputs = [ python38.pkgs.setuptools_scm ];
      preBuild = ''
           export SETUPTOOLS_SCM_PRETEND_VERSION="0.2"
           '';

      meta = {
        homepage = "https://github.com/nmigen/nmigen";
        description = "A refreshed Python toolbox for building complex digital hardware";
      };
    };

@whitequark
Copy link
Member

I might build up to submitting to Nixpkgs, but am just setting up a shell for the moment (I'm a nix noob).

Nixpkgs should already have an nMigen package.

@whitequark
Copy link
Member

export SETUPTOOLS_SCM_PRETEND_VERSION="0.2"

Ah I wasn't aware of that. It would have been nicer to return that instead of "" in your patch, though it's not especially important either way (it only really matters for PyPI I think).

@newhouseb
Copy link
Contributor Author

Ah, I missed that Nixpkgs already has a package, looking now it's basically what I posted, but better (although pinned to a version from April, before the offending code was landed).

My macro use-case is pinning a deterministic set of of newish {lambdasoc, nmigensoc, nmigen, minerva, etc} with my code, since the aforementioned packages are all (understandably) moving targets.

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

Successfully merging this pull request may close these issues.

None yet

2 participants