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 option to override footprint, symbol and 3d model locations #85456

Merged
merged 1 commit into from Oct 17, 2020

Conversation

@matthuszagh
Copy link
Contributor

@matthuszagh matthuszagh commented Apr 17, 2020

Motivation for this change

This PR makes it easy to supply custom library locations, which is useful when working on the libraries or using a git version.

I'm not really sure what the standard nixpkgs practice is here. I like this solution because it makes it really easy to work on the various libraries or use a git version. As I see it, the two main alternatives to this are:

  1. Do it in an overlay.
  2. Do it manually with the "Manage Symbol Libraries..." and "Manage Footprint Libraries...", etc. GUI menu options.

I don't like option 1 because correctly doing this with an overlay does not seem trivial, although maybe someone better at Nix will show me why it's easy. I'm also not a fan of option 2 because the GUI options don't allow you to specify a path that Kicad looks in for the libraries. Rather, you have to specify each library/footprint file individually which is a lot of unnecessary work when doing a lot of work on the kicad libraries. Option 2 also doesn't help when you want to use the git master version of a library.

What are people's thoughts on this?

Things done
  • 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.
@ofborg ofborg bot requested review from berce and Kiwi Apr 17, 2020
@matthuszagh matthuszagh marked this pull request as draft Apr 18, 2020
@matthuszagh matthuszagh marked this pull request as ready for review Apr 18, 2020
@matthuszagh
Copy link
Contributor Author

@matthuszagh matthuszagh commented Apr 18, 2020

Another comment: the whole kicad expression is really tough to override. For instance, changing src does nothing since it's set to base. But base is in a let expression so can't be changed.

So it seems like the easiest way to use a git version or local path for src is to copy the whole expression, edit it to use the local repo, and then call that build expression.

@matthuszagh
Copy link
Contributor Author

@matthuszagh matthuszagh commented Apr 21, 2020

I'm closing this as I'm not convinced my PR is a good idea and my comment without an associated patch isn't super useful. I do think there are problems with this package expression that should be addressed, but I'll hold off until I have some actual changes that others can review. Of course if you disagree feel free to reopen.

@evils
Copy link
Member

@evils evils commented May 2, 2020

huh, not sure why i wasn't added as a reviewer

i did not consider ease of use of mixed components
i suspect it's rare enough that the "do it manually" option is adequate
i think my approach would be to change versions.nix if i wanted to change the library used
#82634 moves libraries and base out of the let block, which may allow such a thing from nix

i overhauled most of the kicad package a while ago but didn't receive much feedback
the previous approach was to create links which left the env var way of overwriting those paths open
this unfortunately tied the compiled source to specific libraries (meaning i had to recompile every time i wanted to try another wrapping related change)
this remains the approach for the i18n because that can't be supplied as an env var

@matthuszagh if you end up reviewing the package, please tag me, or try me on IRC

@matthuszagh
Copy link
Contributor Author

@matthuszagh matthuszagh commented Sep 2, 2020

@evils I'm reopening this, and will fix merge conflicts. I'd like to get your input. I know you mentioned that you would override versions.nix. However, because versions is imported in a let block, I don't see a way to overlay this. Maybe I'm missing something easy though? Additionally, and more importantly in my mind, this still suffers from the library files not being writable.

Adding path arguments, as this PR does, provides a simple way to get either behavior (yours by default). Do you see any significant downside to it?

@evils
Copy link
Member

@evils evils commented Sep 3, 2020

you mentioned that you would override versions.nix. However, because versions is imported in a let block, I don't see a way to overlay this.

i meant my approach if i wanted to mix and match libraries would be to edit the versions.nix file in my nixpkgs checkout (i hope there is a nix native way to change either the content or which file is used, but haven't considered that)
or if i had a local library i wanted to use, add it to kicad via the GUI

this still suffers from the library files not being writable

as long as the library files are distributed by nix, that's always going to be the case?
i don't think the library files are supposed to be writable, my approach if i want to change a something in them is to make add a modified version as a project specific library, which accompanies the project and is writable

Do you see any significant downside to it?

if it fulfills the needs of a user that is not met by the current package, it's probably worth a bit of 'clutter'

@matthuszagh
Copy link
Contributor Author

@matthuszagh matthuszagh commented Sep 3, 2020

Thanks for the feedback @evils!

as long as the library files are distributed by nix, that's always going to be the case?

Agreed, and that makes sense. The nice thing about this PR is kicad doesn't need the actual files, it just needs the path string so doing this doesn't create any purity issues. For instance, I'm using this with flakes and it works fine.

if it fulfills the needs of a user that is not met by the current package, it's probably worth a bit of 'clutter'

Yes, that it does.

evils
evils approved these changes Sep 9, 2020
@matthuszagh matthuszagh force-pushed the kicad-libraries branch 2 times, most recently from f1180a2 to 2bf4060 Oct 10, 2020
@ofborg ofborg bot requested a review from evils Oct 10, 2020
@matthuszagh
Copy link
Contributor Author

@matthuszagh matthuszagh commented Oct 12, 2020

Rebased for this PR.

@matthuszagh
Copy link
Contributor Author

@matthuszagh matthuszagh commented Oct 12, 2020

@doronbehar if you don't mind, it would be great to get your feedback on this PR too.

Copy link
Contributor

@doronbehar doronbehar left a comment

I take back my approval.

@matthuszagh matthuszagh force-pushed the kicad-libraries branch 2 times, most recently from 34830c2 to 5180455 Oct 12, 2020
@matthuszagh
Copy link
Contributor Author

@matthuszagh matthuszagh commented Oct 12, 2020

@doronbehar changes done. Let me know.

@doronbehar
Copy link
Contributor

@doronbehar doronbehar commented Oct 13, 2020

@matthuszagh, this is all wrong, because makeWrapper supports:

# --set-default VAR VAL : like --set, but only adds VAR if not already set in
# the environment

So I'd suggest to make this PR do nothing besides switching the --set arguments to --set-default. Please explain your use case in the commit message.

Previously, these library locations were set absolutely. This
prevented overriding their locations with environment variables. Now,
setting the corresponding environment variable will override the
setting in the environment wrapper. For instance, I can set

KISYSMOD=/some/path/to/footprints

and this will be used as my footprint library instead of the default
footprint library in the nix store. This feature is particularly
useful for having kicad libraries which are writable.
@matthuszagh
Copy link
Contributor Author

@matthuszagh matthuszagh commented Oct 17, 2020

@doronbehar change done.

@doronbehar doronbehar merged commit 1693d00 into NixOS:master Oct 17, 2020
17 checks passed
17 checks passed
@github-actions[bot]
tests
Details
@github-actions[bot]
action
Details
@ofborg[bot]
Evaluation Performance Report Evaluator Performance Report
Details
@github-actions[bot]
Wait for ofborg
Details
@ofborg[bot]
grahamcofborg-eval ^.^!
Details
@ofborg[bot]
grahamcofborg-eval-check-maintainers matching changed paths to changed attrs...
Details
@ofborg[bot]
grahamcofborg-eval-check-meta config.nix: checkMeta = true
Details
@ofborg[bot]
grahamcofborg-eval-darwin nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="9320c69"; rev="9320c69ecb65d25a8eecb32b02652c5ff8443e4d"; } ./pkgs/t
Details
@ofborg[bot]
grahamcofborg-eval-lib-tests nix-build --arg pkgs import ./. {} ./lib/tests/release.nix
Details
@ofborg[bot]
grahamcofborg-eval-nixos nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="9320c69"; rev="9320c69ecb65d25a8eecb32b02652c5ff8443e4d"; } ./nixos/
Details
@ofborg[bot]
grahamcofborg-eval-nixos-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="9320c69"; rev="9320c69ecb65d25a8eecb32b02652c5ff8443e4d"; } ./nixos/
Details
@ofborg[bot]
grahamcofborg-eval-nixos-options nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="9320c69"; rev="9320c69ecb65d25a8eecb32b02652c5ff8443e4d"; } ./nixos/
Details
@ofborg[bot]
grahamcofborg-eval-nixpkgs-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="9320c69"; rev="9320c69ecb65d25a8eecb32b02652c5ff8443e4d"; } ./pkgs/t
Details
@ofborg[bot]
grahamcofborg-eval-nixpkgs-tarball nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="9320c69"; rev="9320c69ecb65d25a8eecb32b02652c5ff8443e4d"; } ./pkgs/t
Details
@ofborg[bot]
grahamcofborg-eval-nixpkgs-unstable-jobset nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="9320c69"; rev="9320c69ecb65d25a8eecb32b02652c5ff8443e4d"; } ./pkgs/t
Details
@ofborg[bot]
grahamcofborg-eval-package-list nix-env -qa --json --file .
Details
@ofborg[bot]
grahamcofborg-eval-package-list-no-aliases nix-env -qa --json --file . --arg config { allowAliases = false; }
Details
@doronbehar
Copy link
Contributor

@doronbehar doronbehar commented Oct 17, 2020

Thanks for your contribution.

@matthuszagh matthuszagh deleted the kicad-libraries branch Oct 17, 2020
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