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

Add FFTW, Bison, M4, Flex, readline, ncurses, Abseil, Google test, NGSPICE, ABC, Netgen, Magic, Icarus Verilog, Icestorm #24

Merged
merged 11 commits into from
Dec 16, 2020

Conversation

per-gron
Copy link
Contributor

They are dependencies of NGSPICE which I intend to add in a follow up commit.

Copy link
Member

@mithro mithro left a comment

Choose a reason for hiding this comment

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

Two random comments.

WORKSPACE Outdated Show resolved Hide resolved
@mithro mithro requested a review from hzeller December 15, 2020 15:23
They are dependencies of NGSPICE which I intend to add in a follow
up commit.
Warnings like these can cause problems if the user of the project
has configured warnings to be treated as errors.
By request from reviewer.
@per-gron
Copy link
Contributor Author

@QuantamHD I rebased the PR and fixed the conflicts. PTAL.

NGSPICE depends on Readline. Readline depends on ncurses. The
smoke tests depend on Absl and Google test.
It's a dependency of Yosys.
Netgen can do LVS checks and more.
This BUILD file builds the non-Tcl version of Magic. It should be useful
for scripting. Adding the Tcl version would be nice but it has difficult
to untangle dependencies to Tk and GUI stuff which is not easy to use in
hermetic builds.
The added dependencies are zlib, gperf and bzip2.
This is used for FPGA synthesis for Lattice ICE40 chips.
@per-gron per-gron changed the title Add FFTW, Bison, M4 and Flex Add FFTW, Bison, M4, Flex, readline, ncurses, Abseil, Google test, NGSPICE, ABC, Netgen, Magic, Icarus Verilog, Icestorm Dec 16, 2020
@per-gron
Copy link
Contributor Author

I added a bunch of more things. Sorry about the huge PR... But in this case I think splitting it up doesn't really make it more reviewable since it's all quite separate.

@per-gron
Copy link
Contributor Author

Once this is reviewed I can continue with Tcl, Yosys, Project Trellis, Nextpnr. That's most of what I have I think.

)

cc_binary(
name = "m4",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Question: would this manual m4 building work with clang/clang++ as well ?

In verible, we build m4 mentioned in our WORKSPACE file, using plain configure_make().

However, when building with clang:

git clone https://github.com/google/verible.git && cd verible
CC=clang CXX=clang++ bazel build //bazel:m4

we run into compile errors apparently having to do with an undefined symbol used by clang's code-generation ?

../lib/libm4.a(xmalloc.o):xmalloc.c:function xnmalloc: error: undefined reference to '__muloti4'
../lib/libm4.a(xmalloc.o):xmalloc.c:function xnrealloc: error: undefined reference to '__muloti4'
../lib/libm4.a(xmalloc.o):xmalloc.c:function xcalloc: error: undefined reference to '__muloti4'

This means, that verible currently can not be built on MacOS (where clang is the default compiler), which limits the audience; So if there is a way to make m4 build reliably on that platform as well, we (@fangism and me) would be very happy to accept guidance (could be in the shape of a pull request :D) how to make this work in verible.

With regard to this pull request, it probably makes sense to test all of these new build rules also with CC=clang CXX=clang++ to make sure we're somewhat compiler-independent.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The BUILD rules in this PR are all copied from a repository that uses clang as the compiler, so the answer is yes but with this caveat:

It works only if the m4 binary is linked with LLVM's compiler-rt (as opposed to libgcc). See android/ndk#295 (comment) and the linked LLVM issue https://bugs.llvm.org/show_bug.cgi?id=16404. The main linker flag you need is -rtlib=compiler-rt. A quick way to get it to work is to build with CC=clang CXX=clang++ bazel build --linkopt=-rtlib=compiler-rt @org_gnu_m4//:m4. Another nice and easy way of setting up a clang toolchain is to use this LLVM Bazel toolchain which easily gives you a nearly (it doesn't disable global system header include paths) hermetic toolchain.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Once I've pulled in the deps that I'm working on now I'd be happy to set up CI that runs Clang and GCC. @mithro could maybe pull some strings to give me access to a GCP project that has Cloud Build enabled? I have a feeling that the CI solutions that can be had for free for FOSS projects are not quite going to be fast enough to be nice to use.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there a way in a bazel BUILD file to automatically enable compiler-rt depending on the toolchain used ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@hzeller Last time I checked Bazel did not offer a way to select based on GCC vs Clang. It's really weird to me that they don't offer this, it's really quite common to have to choose copts based on this. I think you could select based on the OS, see https://github.com/tensorflow/tensorflow/blob/master/third_party/clog/BUILD.bazel

Since this is arguably a bug in the toolchain the "proper" way to fix it (short of fixing LLVM) is to change the Bazel toolchain configuration, which is what using https://github.com/grailbio/bazel-toolchain does for you, although that also downloads LLVM binaries.

Copy link
Collaborator

@hzeller hzeller left a comment

Choose a reason for hiding this comment

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

LGTM

@per-gron
Copy link
Contributor Author

I don't have merge access to this repo, so if someone can push the merge button that'd be good. Then I'd be able to make a fresh follow-up PR without adding to this insanely large one 😄

@hzeller hzeller merged commit 69c11fe into hdl:main Dec 16, 2020
@per-gron per-gron deleted the m4-bison-flex-fftw branch December 16, 2020 23:02
@mithro
Copy link
Member

mithro commented Dec 16, 2020

@per-gron - Your access level should now be fixed.

@hzeller hzeller mentioned this pull request Dec 17, 2020
@jesec jesec mentioned this pull request Jun 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants