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

ghdl: fix llvm backend, add passthru.tests #101423

Merged
merged 1 commit into from Apr 17, 2021
Merged

Conversation

@wamserma
Copy link
Contributor

@wamserma wamserma commented Oct 22, 2020

Motivation for this change

Closes #97466

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.
@omasanori
Copy link
Contributor

@omasanori omasanori commented Oct 23, 2020

Result of nixpkgs-review pr 101423 1

2 packages built:
  • ghdl (ghdl-mcode)
  • ghdl-llvm

Copy link
Contributor

@Lucus16 Lucus16 left a comment

Your postInstall hack won't be accepted in nixpkgs. With Nix, compilation and linking is always tricky because the tools assume files can be found in standard places. The generally agreed upon solution is to always use a nix shell. It's a good habit to create a shell.nix for each project you work on because it lets you specify dependencies for each project independently. It can be a bit annoying to enter nix-shell every time but that's how it is. In this case, something like this would suffice:

with import <nixpkgs> { };
mkShell { buildInputs = [ ghdl ]; }

That said, thanks a lot for catching that zlib should indeed be in propagatedBuildInputs and for writing a test! I cleaned up a bit (see my other comments) and pushed it to https://github.com/Lucus16/nixpkgs/tree/ghdl-llvm/pkgs/development/compilers/ghdl

pkgs/development/compilers/ghdl/test-simple.nix Outdated Show resolved Hide resolved
pkgs/development/compilers/ghdl/test-simple.nix Outdated Show resolved Hide resolved
pkgs/development/compilers/ghdl/test-simple.nix Outdated Show resolved Hide resolved
@wamserma
Copy link
Contributor Author

@wamserma wamserma commented Oct 23, 2020

Your postInstall hack won't be accepted in nixpkgs.

Awwww, i really found it unnerving that it was not working out-of-the box like ghdl-mcode and once logic gets more complex the llvm-backend is much faster.
But you are right, such projects deserve a shell.nix anyway.
How about a wrapper that checks whether ghdl-llvm is launched from a nix-shell and prints a hint to use nix-shell otherwise?

That said, thanks a lot for catching that zlib should indeed be in propagatedBuildInputs and for writing a test! I cleaned up a bit (see my other comments) and pushed it to https://github.com/Lucus16/nixpkgs/tree/ghdl-llvm/pkgs/development/compilers/ghdl

Thanks, looks good and I'll cherry-pick your updates onto the PR, if it's ok for you.

@Lucus16
Copy link
Contributor

@Lucus16 Lucus16 commented Oct 24, 2020

How about a wrapper that checks whether ghdl-llvm is launched from a nix-shell and prints a hint to use nix-shell otherwise?

This would not be a complete solution since someone may find other ways to allow linking with libz. I think the best solution would be to create a simple page for ghdl on the Nixos wiki at https://nixos.wiki/ and/or create a small ghdl section in the nixpkgs manual.

Thanks, looks good and I'll cherry-pick your updates onto the PR, if it's ok for you.

By all means.

@wamserma
Copy link
Contributor Author

@wamserma wamserma commented Mar 20, 2021

@Lucus16 Finally got around to updating this PR with your suggestions.
CC @thoughtpolice as the list of maintainers has grown since.

@wamserma
Copy link
Contributor Author

@wamserma wamserma commented Mar 25, 2021

@wamserma
Copy link
Contributor Author

@wamserma wamserma commented Apr 17, 2021

ping @Lucus16 @SuperSandro2000 can we get this done?

Copy link
Member

@thoughtpolice thoughtpolice left a comment

Sorry for the delay on this; LGTM, thank you!

@thoughtpolice thoughtpolice merged commit e9254e1 into NixOS:master Apr 17, 2021
21 checks passed
@wamserma wamserma deleted the ghdl-llvm branch Apr 19, 2021
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.

5 participants