Enable cross compiling support without patches #1749
Merged
+339
−209
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1749 +/- ##
=======================================
Coverage 73.23% 73.23%
=======================================
Files 381 381
Lines 54343 54343
Branches 9246 9246
=======================================
Hits 39796 39796
Misses 11573 11573
Partials 2974 2974 ☔ View full report in Codecov by Sentry. |
bryanwweber
force-pushed
the
bryan-remove-patch-for-cross-compiling
branch
3 times, most recently
from
9a091a8 to
984488b
Compare
bryanwweber
commented
Aug 7, 2024
bryanwweber
force-pushed
the
bryan-remove-patch-for-cross-compiling
branch
6 times, most recently
from
4d249f6 to
4369f84
Compare
bryanwweber
force-pushed
the
bryan-remove-patch-for-cross-compiling
branch
from
433c941 to
4d060bd
Compare
ischoegl
reviewed
Aug 14, 2024
ischoegl
reviewed
Aug 15, 2024
There was a problem hiding this comment.
This PR is somewhat out of my wheelhouse, but as far as I can tell it looks overall good to me (and the CI passes!).
I have two trivial comments about buildutils.py ... we may want to start being consistent with PEP 484 for additions as it really takes out the guesswork about what those parameters are to be expected?
|
Thanks @ischoegl! Good suggestions, I added them 😄 |
bryanwweber
force-pushed
the
bryan-remove-patch-for-cross-compiling
branch
from
7ac6ae8 to
1a66bb7
Compare
speth
requested changes
Aug 18, 2024
There was a problem hiding this comment.
Thanks, @bryanwweber. It will be really nice to be able to avoid the painfully slow emulator-based builds. I had just a couple of small comments.
Putting the definition in buildutils lets other functions in buildutils use config_error
Introduce a function to run the C preprocessor. This can be used to output values of #defined variables in header files without actually running a program.
Since we can run the C preprocessor to find version information about Sundials, we can simplify many of the checks we have when the system Sundials is specified. In particular, we can determine ahead of time which calls we expect to find and which libraries to use, without having to compile each one. In addition, the Sundials check is refactored into buildutils to simplify SConstruct
Why was this even implemented for apple clang?
We don't need defines in there anymore.
The setting previously didn't correctly check for mingw in the toolchain variable.
If the text is empty we don't want to process even if the return was successsful
Other configuration relies on this being set to 'n' when we're using the private Sundials submodule
Versions of fmtlib compiled with Unicode support require the /utf-8 flag
bryanwweber
force-pushed
the
bryan-remove-patch-for-cross-compiling
branch
from
1a66bb7 to
36e3cf4
Compare
bryanwweber
force-pushed
the
bryan-remove-patch-for-cross-compiling
branch
from
36e3cf4 to
250bc53
Compare
|
@speth Comments are addressed, thanks for the review! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Changes proposed in this pull request
#defined values.If applicable, fill in the issue number this pull request is fixing
Closes Cantera/enhancements#210
If applicable, provide an example illustrating new features this pull request is introducing
Checklist
scons build&scons test) and unit tests address code coverage