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

Improved CMake configuration #156

Merged
merged 8 commits into from
Feb 12, 2023
Merged

Improved CMake configuration #156

merged 8 commits into from
Feb 12, 2023

Conversation

arengarajan99
Copy link
Collaborator

Currently, there are still files that require includes of .c files, which is not ideal since we already use CMake. This pull request addresses this issue.

Copy link
Member

@lhstrh lhstrh left a comment

Choose a reason for hiding this comment

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

LGTM!

@erlingrj
Copy link
Collaborator

erlingrj commented Feb 8, 2023

This is again back to the discussion on how much we want to depend on CMake. If we want to support other build tools then we need to reimplement this logic. Using the preprocessor avoids this, we still have to make the compile definitions though. But what are the examples of other build systems. @petervdonovan do you have a comment? Was it based on the nRF52 thing?

@arengarajan99
Copy link
Collaborator Author

arengarajan99 commented Feb 8, 2023

Part of the reason why I need to convert it back is based on how federated files having includes of .c files which messes with arduino compilation since things get compiled multiple times. I recall discussing with peter about having sole reliance on CMake, but in instances where cmake is not possible, still enforce the C standard of including headers and never including .c files under any circumstance within files. I could keep threaded includes the same since its a single file, but it seems rather weird to break from c norms for the sake of diversifying build systems.

@erlingrj
Copy link
Collaborator

erlingrj commented Feb 8, 2023

We could still use the preprocessor, just have #ifdefs in all the different schedulers and have them all be used as source files for the built. Before we do that I think we should reiterate on the reasons for not wanting to go all-in with CMake and see if the assumptions are still valid, so waiting to hear what Peter has to say there.

Btw, it is not in the C standard that you should not include source files from other source files, it is not recommended, but if you know what you are doing then it can be a useful tool.

@petervdonovan
Copy link
Contributor

petervdonovan commented Feb 8, 2023

This is again back to the discussion on how much we want to depend on CMake. If we want to support other build tools then we need to reimplement this logic. Using the preprocessor avoids this

100% agree

I recall discussing with peter about having sole reliance on CMake

I have said that, but that was based on an (apparently naive) hope that CMake would be cross-platform enough for all our needs. Since then my opinion has shifted based on discussion with you and Erling and Martin. I am now fully on board with the idea of using the preprocessor (instead of CMake) to handle conditional code inclusion.

(Edit: To clarify, I never really preferred CMake over the preprocessor per se. I preferred CMake over an ad hoc Java-based build system the implementation of which was spread through several parts of our monolithic code base.)

but in instances where cmake is not possible, still enforce the C standard of including headers and never including .c files under any circumstance within files.

it is not in the C standard that you should not include source files from other source files, it is not recommended, but if you know what you are doing then it can be a useful tool.

I still do not like #including .c files, although I could be convinced otherwise. Previously, we got lots of weird errors due to this practice from the C++ compiler, which apparently did not like #including .c files. To make the C++ compiler happy, I decided to avoid #inclusion of .c files like the plague. We no longer compile the C runtime with a C++ compiler (thank goodness), even in the CCpp target (we just compile user-supplied code and generated code as C++), but from this experience I still perceive #inclusion of .c files as hacky and nonstandard, even if as Erling says there is nothing in the C standard that forbids it.


We could still use the preprocessor, just have #ifdefs in all the different schedulers and have them all be used as source files for the built.

Agreed! I like this solution. It factors logic into the C preprocessor, which is the most cross-platform build tool that I could imagine, and it avoids #inclusion of .c files.

@arengarajan99
Copy link
Collaborator Author

Yeah the C Standard itself doesn't preclude the use of includes, but it does seem extremely hacky. I agree as well that the preprocessor definitions inside the .c files will solve the majority of our issues

@erlingrj
Copy link
Collaborator

erlingrj commented Feb 9, 2023

I just remembered now that #if SCHEDULER == ADAPTIVE etc. can ONLY be used after lf_types.h has been included. Because that is where ADAPTIVE, GEDF etc is defined to be integers. I could not really come up with a better fix here. We could also pass ADAPTIVE=1, GEDF=2 when invoking the build tool

@lhstrh
Copy link
Member

lhstrh commented Feb 12, 2023

@erlingrj and @petervdonovan is this good to go or are you still requesting changes?

Copy link
Contributor

@petervdonovan petervdonovan left a comment

Choose a reason for hiding this comment

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

LGTM

@lhstrh lhstrh changed the title Tweak Federated and Threaded Runtimes to give control back to CMake Improved CMake configuration Feb 12, 2023
@lhstrh lhstrh merged commit 836aa3c into main Feb 12, 2023
@lhstrh lhstrh deleted the fed-include-fix branch February 12, 2023 06:27
@lhstrh lhstrh added the enhancement Enhancement of existing feature label Feb 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Enhancement of existing feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants