NOhs commented 4 years ago

The compiler now has a compile and link feature so the calls to clang are all in that class and not distributed across different files and classes.

NOhs commented 4 years ago

This is based on the branch of the improved C++ standard detection and should only be merged after that branch.

codecov[bot] commented 4 years ago

Codecov Report

Merging #118 into master will increase coverage by 0.40%. The diff coverage is 85.88%.

@@            Coverage Diff             @@
##           master     #118      +/-   ##
==========================================
+ Coverage   79.55%   79.96%   +0.40%     
==========================================
  Files          17       16       -1     
  Lines        1218     1153      -65     
==========================================
- Hits          969      922      -47     
+ Misses        249      231      -18

Impacted Files	Coverage Δ
clang_build/directories.py	`100.00% <ø> (ø)`
clang_build/project.py	`84.56% <ø> (ø)`
clang_build/toolchain.py	`82.29% <82.29%> (ø)`
clang_build/target.py	`80.26% <83.33%> (-0.35%)`	:arrow_down:
clang_build/environment.py	`88.88% <100.00%> (ø)`
clang_build/flags.py	`100.00% <100.00%> (ø)`
clang_build/io_tools.py	`100.00% <100.00%> (ø)`
clang_build/single_source.py	`63.26% <100.00%> (+14.05%)`	:arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 92b69fd...813dfc6. Read the comment docs.

GPMueller commented 4 years ago

The MacOS build seems to fail due to incorrect bundling flags:

[project.mylib]: link -> "/Users/travis/build/[secure]/clang-build/build/mylib/default/lib/libmylib.dylib"
Linking was unsuccessful:
Target [project.mylib] did not link. Errors:
clang-10: error: unknown argument: '-install_name @rpath/libmylib.dylib'

I currently cannot find a reason why this appears only on this branch...

In platform.py we have PLATFORM_BUNDLING_LINKER_FLAGS = ["-Wl,-rpath,@executable_path"], while in target.py in SharedLibrary we have self.compile_commands += [f"-install_name @rpath/{self.outfilename}"]. According to this SO answer, install_name should be passed as a compile flag, not a link flag - but since it previously used to work, this flag might actually not be required?

NOhs commented 4 years ago

Moved the "-std=c++XX" to the flags class. Not sure if better. Everything is getting way to entangled again, smells like some structural optimization is still required.

GPMueller commented 4 years ago

I am currently trying around with creating an Emscripten toolchain class (this arguably falls under the topic of cross-compilation) and noticed:

the DEFAULT_COMPILE_FLAGS etc. in flags.py should be part of Toolchain, as they might be different for different compilers
the platform defaults, such as EXECUTABLE_SUFFIX should also be part of Toolchain (for emscripten the suffix would be '.js')
on another PR
- maybe we should provide a Toolchain base class (like we do with the TargetDescription class) from which users can derive in order to specialise behaviour for their toolchain?
- we should make it possible to select a different toolchain somehow, maybe by the user providing a toolchain.py file

See https://mesonbuild.com/Cross-compilation.html for some excellent explanations about cross-compilation.

GPMueller commented 4 years ago

Another thing I would like to change about the current implementation on this PR, though I'm not sure how to cleanly do it: log messages of the Toolchain do not contain the project or target tag at the beginning. Especially when there are several compile commands (even more so when they are run in parallel), it is easier to read when there are [project.target] tags in front of the messages.

Maybe the toolchain functions should take a target parameter and call it's logger?

Trick-17 / clang-build

Improved compiler #118

Codecov Report