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

kicad: add srcs parameter to allow configuring kicad versions #100066

Merged
merged 6 commits into from Oct 12, 2020

Conversation

@matthuszagh
Copy link
Contributor

@matthuszagh matthuszagh commented Oct 9, 2020

Motivation for this change

Allows overriding kicad versions which are otherwise inaccessible.

Things done

Added srcs parameter that can be used to customize kicad or kicad-unstable versions, including libraries. For instance, you could do something like:

final: prev:

{
  kicad-unstable = (prev.kicad-unstable.override {
    srcs = {
      "kicad-unstable" = {
        kicadVersion = {
          version = "2020-10-08";
          src = {
            rev = "fd22fe8e374ce71d57e9f683ba996651aa69fa4e";
            sha256 = "sha256-F8qugru/jU3DgZSpQXQhRGNFSk0ybFRkpyWb7HAGBdc=";
          };
        };
        libVersion = {
          version = "2020-08-22";
          libSources = {
            i18n.rev = "cbbb1efd940094bf0c3168280698b2b059a8c509";
            i18n.sha256 = "1q4jakn6m8smnr2mg7jgb520nrb6fag9mdvlcpx3smp3qbxka818";
            symbols.rev = "9ca6a5348cdeb88e699582d4ed051ff7303b44d3";
            symbols.sha256 = "13w6pb34rhz96rnar25z7kiscy6q1fm8l39hq1bpb8g9yn86ssz4";
            templates.rev = "ae16953b81055855bcede4a33305413599d86a15";
            templates.sha256 = "1pkv90p3liy3bj4nklxsvpzh9m56p0k5ldr22armvgqfaqaadx9v";
            footprints.rev = "f94c2d5d619d16033f69a555b449f59604d97865";
            footprints.sha256 = "1g71sk77jvqaf9xvgq6dkyvd9pij2lb4n0bn0dqnwddhwam935db";
            packages3d.rev = "f699b0e3c13fe75618086913e39279c85da14cc7";
            packages3d.sha256 = "0m5rb5axa946v729z35ga84in76y4zpk32qzi0hwqx957zy72hs9";
          };
        };
      };
    };
}

I've also reformatted the existing code. Let me know if the reformatting is too invasive and I'll revert.

  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.
@doronbehar
Copy link
Contributor

@doronbehar doronbehar commented Oct 9, 2020

@matthuszagh this change is somewhat simple enough so it's easy for me to press the button, but I think it would be nice if it was possible to use a different fork of a library / kicad itself, not only a different revision.

P.S the eval errors have nothing to do with you.

@matthuszagh
Copy link
Contributor Author

@matthuszagh matthuszagh commented Oct 9, 2020

I didn't consider that but I agree with you. Let me dig into this a little more.

@matthuszagh
Copy link
Contributor Author

@matthuszagh matthuszagh commented Oct 10, 2020

@doronbehar this now allows overriding with a full fetchFromGitLab, or whatever you want. Versions can also be overriden with kicadVersion or libVersion. For example, this is how I'm using it:

final: prev:

{
  kicad-unstable = (prev.kicad-unstable.override {
    srcs = {
      kicadVersion = "2020-10-08";
      kicad = prev.fetchFromGitLab {
        group = "kicad";
        owner = "code";
        repo = "kicad";
        rev = "fd22fe8e374ce71d57e9f683ba996651aa69fa4e";
        sha256 = "sha256-F8qugru/jU3DgZSpQXQhRGNFSk0ybFRkpyWb7HAGBdc=";
      };
    };
  });
}

Let me know what you think.

Copy link
Contributor

@doronbehar doronbehar left a comment

Besides these, here are general comments:

  1. It'd be easier to review this if the indentation settings will be in a separate commit.
  2. More comments to how the logic is going on inside default.nix regarding srcs.
  3. Since there's no way to query a package's override options, it'd be nice to put a big comment at the start of default.nix with all the possible ways one can override srcs.

pkgs/applications/science/electronics/kicad/libraries.nix Outdated Show resolved Hide resolved
pkgs/applications/science/electronics/kicad/i18n.nix Outdated Show resolved Hide resolved
pkgs/applications/science/electronics/kicad/default.nix Outdated Show resolved Hide resolved
@doronbehar
Copy link
Contributor

@doronbehar doronbehar commented Oct 10, 2020

Oh and 1 more thing: I'm rather sure this change shouldn't trigger rebuilds - ofborg detects it this way and in the "eval" check of the PR, there's a link to the list of changed outputs. Also the PR's labels ofborg sets reflects that.

@matthuszagh
Copy link
Contributor Author

@matthuszagh matthuszagh commented Oct 10, 2020

@doronbehar thanks for the review. I've applied the changes. Let me know.

Copy link
Contributor

@doronbehar doronbehar left a comment

The review is a bit messy sorry, but the expression is getting better and better :). Some suggestions are better of in separate commits.

pkgs/applications/science/electronics/kicad/base.nix Outdated Show resolved Hide resolved
pkgs/applications/science/electronics/kicad/base.nix Outdated Show resolved Hide resolved
pkgs/applications/science/electronics/kicad/base.nix Outdated Show resolved Hide resolved
pkgs/applications/science/electronics/kicad/base.nix Outdated Show resolved Hide resolved
pkgs/applications/science/electronics/kicad/default.nix Outdated Show resolved Hide resolved
pkgs/applications/science/electronics/kicad/default.nix Outdated Show resolved Hide resolved
@matthuszagh
Copy link
Contributor Author

@matthuszagh matthuszagh commented Oct 10, 2020

@doronbehar I hope I got everything (I think I did), but sorry in advance if I messed something up (got into a little trouble with all the rebasing :)).

Also let me know about those other two points.

Copy link
Contributor

@doronbehar doronbehar left a comment

Not many more to go. The commit messages look good overall. Only commit 239d16c should mention the rename of the *Support arguments to with*.

pkgs/applications/science/electronics/kicad/base.nix Outdated Show resolved Hide resolved
pkgs/applications/science/electronics/kicad/default.nix Outdated Show resolved Hide resolved
pkgs/applications/science/electronics/kicad/base.nix Outdated Show resolved Hide resolved
@matthuszagh
Copy link
Contributor Author

@matthuszagh matthuszagh commented Oct 11, 2020

I've implemented the changes. Let me know if there's anything else.

Copy link
Contributor

@doronbehar doronbehar left a comment

Thanks for your cooperation. Only issues I see are in the assertions commit. It's almost perfect.

pkgs/applications/science/electronics/kicad/base.nix Outdated Show resolved Hide resolved
pkgs/applications/science/electronics/kicad/base.nix Outdated Show resolved Hide resolved
pkgs/applications/science/electronics/kicad/base.nix Outdated Show resolved Hide resolved
@matthuszagh
Copy link
Contributor Author

@matthuszagh matthuszagh commented Oct 12, 2020

I've made the changes, let me know what you think @doronbehar.

Copy link
Contributor

@doronbehar doronbehar left a comment

Also, to be nice to everyone else, we should add release notes to rl-2103.xml (ideally in seperate commit from this one).

The commit message of this "rename options commit" should mention also the fact that the defaults are not needed in base.nix, although it doesn't affect users directly.

1 last round I promise!

pkgs/applications/science/electronics/kicad/default.nix Outdated Show resolved Hide resolved
@doronbehar
Copy link
Contributor

@doronbehar doronbehar commented Oct 12, 2020

I know it ruins the git commit --fixup= workflow, but could you please mention in that commit the fact that base.nix doesn't need the defaults to be specified in it?

Also, we'll definitely need a commit that mentions these api changes in rl-2103.xml.

matthuszagh added 3 commits Oct 12, 2020
Also: Use assertions instead of silently ignoring arguments that don't cooperate
(occ+oce) / won't compile (aarch64 + oce).

base.nix no longer provides default argument values since these are
provided by default.nix.
This also exposes the full src and version parameters for each
derivation, allowing them to overrideable by srcs.
@matthuszagh
Copy link
Contributor Author

@matthuszagh matthuszagh commented Oct 12, 2020

Changes in.

Copy link
Contributor

@doronbehar doronbehar left a comment

Thanks for your contribution.

@doronbehar doronbehar merged commit 58b8ae4 into NixOS:master Oct 12, 2020
17 checks passed
@matthuszagh matthuszagh deleted the kicad-srcs branch Oct 12, 2020
Copy link
Member

@evils evils left a comment

sorry for such a late review
i missed it because i confused its mails for #85456
and it being merged just 3 days after being opened did not leave a lot of time to discover that mistake


# oce on aarch64 fails a test
withOCE = oceSupport && !stdenv.isAarch64;
withOCC = (withOCCT && !withOCE) || (oceSupport && stdenv.isAarch64);
Copy link
Member

@evils evils Jan 24, 2021

it took me quite a long time figuring this out
can you prove that what you used instead is equivalent?

}:

assert ngspiceSupport -> libngspice != null;
# The `srcs` parameter can be used to override the kicad source code
Copy link
Member

@evils evils Jan 24, 2021

having never used overlays, i did not write this package with them in mind
my original intent was for the versions.nix being the single reference for all version info
i don't entirely get your changes, that's ok (but because of that i'd like you to be added to meta.maintainers)

but would it not be simpler to just provide kicad with a different versions.nix (with which i mean the nix content of that file) (or for example a way to merge updates to it so one does not have to provide a full replacement)?

right now i'm assuming that's not possible
or that i am missing a benefit to this more complicated approach

let
mkLib = name:
stdenv.mkDerivation {
pname = "kicad-${name}";
# Use the revision instead of `version` (which is an ISO 8601 date)
# to prevent duplicating the library when just the date changed
version = "${builtins.substring 0 10 libSources.${name}.rev}";
Copy link
Member

@evils evils Jan 24, 2021

this was a rev substring for a very good reason
with it being the date (for kicad-unstable), the 3D models get downloaded and "built" again when only the date, but nothing else, changed
since this is a ~6gb download, i strongly believe it is worth avoiding if at all possible (which it is)

(i suspect this PR causes this to now be the date, but i can't actually tell...)


versions = import ./versions.nix;
versionConfig = versions.${baseName};
libSrcFetch = name: fetchFromGitLab {
Copy link
Member

@evils evils Jan 24, 2021

one of the goals of my rewrite was to simplify default.nix to be as much as possible just the wrapper
code related to building the components that get tied together here should be in base.nix or libraries.nix respectively

@@ -42,30 +28,4 @@ in
templates = mkLib "templates";
footprints = mkLib "footprints";
packages3d = mkLib "packages3d";

# i18n is a special case, not actually a library
Copy link
Member

@evils evils Jan 24, 2021

i think this description of i18n being optional, separate and only found via a relative path should have been retained

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

Successfully merging this pull request may close these issues.

None yet

3 participants