search

jacobly0 / llvm-project

This fork of the canonical git mirror of the LLVM subversion repository adds (e)Z80 targets. Please refer to the wiki for important build instructions.
https://github.com/jacobly0/llvm-project/wiki
123 stars 15 forks source link

Following README.md instructions doesn't compile anything targetting Z80 #35

Closed cpcitor closed 1 year ago

cpcitor commented 1 year ago

Hi, author of https://github.com/cpcitor/cpc-dev-tool-chain here

Interesting work you did here!

Selecting the Z80 branch then following the README.md, I was somehow surprised of not seeing anything related to Z80 in the compiled binaries.

I was surprised to not see this in the (open or closed) issues in this repo. Perhaps the llvm-specific cmake options are just obvious to people that have already compiled clang for cross compiling to experimental targets?

Anyway, I found some hints reading https://www.cocoacrumbs.com/blog/2021-12-28-experimenting-with-the-llvm-toolchain-for-the-ez80/ which mentioned https://bitbucket.org/cocoacrumbselectronics/ez80-llvm-toolchain/src/master/setupToolchain.sh which contained this:

cmake ../llvm -GNinja -DLLVM_ENABLE_PROJECTS="clang" \
    -DCMAKE_INSTALL_PREFIX=$INSTALLDIR \
    -DCMAKE_BUILD_TYPE=Release \
    -DLLVM_EXPERIMENTAL_TARGETS_TO_BUILD=Z80 \
    -DLLVM_TARGETS_TO_BUILD= \
    -DLLVM_DEFAULT_TARGET_TRIPLE=ez80-none-elf

IMHO this deserves an updated README.md with specific instructions to enable compilation of the Z80 target.

I may offer a PR, yet you might have more precise instructions or content for that file?

adriweb commented 1 year ago

You may find useful info here: https://github.com/jacobly0/llvm-project/wiki

Basically, symlink/rename the binary so that the expected target is the default (for instance ez80-clang), or pass -target xxxx to the clang binary.

cpcitor commented 1 year ago

Ah, thanks!

Still, the README.md is misleading. Are you open to a PR? Something as simple as below should do.

This is a fork of llvm-project (...) Please visit the wiki for build instructions and more: https://github.com/jacobly0/llvm-project/wiki

Also perhaps at least the repository summary (right column) should be different from this:

About

This is the canonical git mirror of the LLVM subversion repository. The repository does not accept github pull requests at this moment. Please submit your patches at http://reviews.llvm.org/. llvm.org

mateoconlechuga commented 1 year ago

This is a fork of the llvm project so it can be upstreamed - it shouldn't really have any specific target instructions in the readme or otherwise.

cpcitor commented 1 year ago

README.md : understood

This is a fork of the llvm project so it can be upstreamed - it shouldn't really have any specific target instructions in the readme or otherwise.

Okay, I got it. You'd like that when merging back the changes into LLVM at some point in the future, there's no need to resolve conflict on the content of README.md. This choice make it so that just plain merge should work.

That might be arguable, but I'm okay with it.

That answers the point about README.md.

Repo description: different

On the contrary, the "about" section (right column in the repo view) is different. Currently it reads:

This is the canonical git mirror of the LLVM subversion repository. The repository does not accept github pull requests at this moment. Please submit your patches at http://reviews.llvm.org/.

The sentence above:

E.g.

This fork of the canonical git mirror of the LLVM subversion repository is intended for the Z80 target and might be merged back to it some day. Please refer to https://github.com/jacobly0/llvm-project/wiki for Z80-specific build instructions.

You get the best of both worlds: clean upstreaming, clear welcome message for newcomers.

I can't submit a PR for this, as it's not part of the code but part of the repo metadata.

What do you think?

adriweb commented 1 year ago

Let's just make jacobly decide, but he's pretty busy already and there are more importent changes to merge and work on :p