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

Jump to bottom

Update FOSS EDA tools again #67022

Merged
merged 9 commits into from Aug 23, 2019
Merged

Update FOSS EDA tools again #67022

merged 9 commits into from Aug 23, 2019

Conversation

emilazy
Copy link
Member

@emilazy emilazy commented Aug 20, 2019

Motivation for this change

nextpnr was broken due to the SHA-256 of an incorrect commit being used, oops; so I took the opportunity to bump versions again, fix a few issues, and add myself to maintainers. (It was working without errors for me locally because of fetchFromGitHub using a fixed-output derivation and having the other commit cached; is there any way to proactively debug these issues?)

I also extended the platforms to platforms.all because all of these tools should work fine on Darwin/macOS as far as I know (in fact, they all run on Windows). Let me know if this is incorrect.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • 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 nix-review --run "nix-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.
Notify maintainers

cc @thoughtpolice; I'm pretty sure the reason the nextpnr GUI was broken was overriding qtbase rather than using libsForQt511, but I can't reproduce the issue with Qt 5.12 locally anyway so I bumped it back. It looks like it might be related to YosysHQ/nextpnr#240 / YosysHQ/nextpnr#278; YosysHQ/nextpnr#169 might have fixed it?

cc @worldofpeace for the Qt nixpkgs infra changes in 993c02afd472de0741cb5defa9e81440438c76c1

@ofborg ofborg bot requested a review from thoughtpolice August 20, 2019 05:19
thoughtpolice
thoughtpolice approved these changes Aug 20, 2019
View reviewed changes
Copy link
Member

@thoughtpolice thoughtpolice left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for very much for this! Running a build on my machine (which should finish soon), but these all look good to me. I'm glad to see the Trellis hack (which required a 'double build' due to the overridden postInstall) is gone, and very happy to see the NextPNR GUI finally enabled!

@thoughtpolice
Copy link
Member

thoughtpolice commented Aug 20, 2019 •
edited

@GrahamcOfBorg build icestorm trellis

(This is to check x86_64-darwin compatibility since the platforms were expanded.)

@thoughtpolice thoughtpolice self-assigned this Aug 20, 2019
@thoughtpolice
Copy link
Member

Trellis builds just fine on Darwin, but I'm afraid icestorm does not: it requires libftdi, which does not build on Darwin at the moment. However, I think simply tweaking the meta of libftdi alone might be enough to succeed, since libusb already works anyway. (The reasoning it's not enabled was likely the same I made with IceStorm/Trellis/Yosys -- I simply conservatively scoped them to Linux-only, since until very recently I did not have a Darwin machine available.)

worldofpeace
worldofpeace reviewed Aug 20, 2019
View reviewed changes
pkgs/top-level/all-packages.nix
nextpnr = libsForQt5.callPackage ../development/compilers/nextpnr {
# QT 5.12 has a weird regression involving the floorplanning window having
# a 'blank' or 'transparent' background, so fall back to 5.11 for now.
qtbase = qt511.qtbase;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

note: The proper way to have done that would have been to use libsForQt511.callPackage

@emilazy
Copy link
Member Author

emilazy commented Aug 20, 2019

Hopefully it should build on Darwin now; I unfortunately don't have a machine to hand to test with so we'll have to see what borg thinks again.

@ofborg ofborg bot requested a review from thoughtpolice August 20, 2019 18:21
@thoughtpolice
Copy link
Member

@GrahamcOfBorg build icestorm

@thoughtpolice
Copy link
Member

It looks like we'll also need to make --with-async-mode in libftdi optional and turn it off for darwin:

https://logs.nix.ci/?key=nixos/nixpkgs.67022&attempt_id=033bb25f-0eea-40c3-a82d-f483ee994a31

ftdi.c:44:10: fatal error: 'linux/usbdevice_fs.h' file not found
#include <linux/usbdevice_fs.h>
         ^~~~~~~~~~~~~~~~~~~~~~
1 error generated.

see:

https://github.com/nblock/libftdi/blob/d0fc6f3f412090025bf8d4a07dec0f55466f457a/configure.in#L37

and

https://github.com/nblock/libftdi/blob/d0fc6f3f412090025bf8d4a07dec0f55466f457a/src/ftdi.c#L39

A simple configureFlags = optional (!stdenv.isDarwin) [ "--with-async-mode" ]; should probably do the trick.

@thoughtpolice
Copy link
Member

(Also note: we can also just revert the changes to the platforms settings for now, if you'd like me to merge this sooner instead of us playing whack-a-mole with borg, and we can add darwin support for libftdi/icestorm in another PR)

@emilazy
Copy link
Member Author

emilazy commented Aug 20, 2019 •
edited

I'm okay doing whack-a-mole to get the packages more broadly supported; all my local stuff has pinned versions currently anyway. Hopefully the asynchronous mode isn't required by icestorm...

@emilazy emilazy force-pushed the update-eda-tools-again branch 5 times, most recently from a6a7b2d to 9deb786 Compare August 21, 2019 01:33
@thoughtpolice
Copy link
Member

@GrahamcOfBorg build icestorm

@thoughtpolice
Copy link
Member

Good news: looks like libftdi should work fine on Darwin and it looks like the build succeeded! Bad news: pypy does not work. :( But it looks like that's the last broken dependency for icestorm on Darwin.

We might just want to change the icestorm pypy conditional for now to be usePyPy ? (stdenv.isx86_64 && !stdenv.isDarwin), which should unblock this. Personally, I'd prefer to do this rather than add PyPy for macOSX in this PR, mainly because getting it working is more important than it being extremely fast to build, and we can always tidy that up later on.

@emilazy
Copy link
Member Author

emilazy commented Aug 22, 2019

Switched over the icestorm conditional to the more specific usePyPy ? stdenv.hostPlatform.system == "x86_64-linux", hopefully that should fix things :)

@thoughtpolice
Copy link
Member

@GrahamcOfBorg build icestorm

@emilazy
Copy link
Member Author

emilazy commented Aug 23, 2019

yay!

@thoughtpolice thoughtpolice merged commit 6dba65e into NixOS:master Aug 23, 2019
@thoughtpolice
Copy link
Member

@emilazy Merged, and thanks so much for these updates and contributing!

@emilazy emilazy deleted the update-eda-tools-again branch August 23, 2019 20:31
@emilazy emilazy mentioned this pull request Feb 10, 2020
Merged
10 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@worldofpeace worldofpeace worldofpeace left review comments

@thoughtpolice thoughtpolice Awaiting requested review from thoughtpolice

Assignees

@thoughtpolice thoughtpolice

Labels
8.has: package (new) 10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10 11.by: package-maintainer
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@emilazy @thoughtpolice @worldofpeace