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

Tracing tools Makefile fixed #1485

Merged
merged 1 commit into from
Nov 22, 2022
Merged

Tracing tools Makefile fixed #1485

merged 1 commit into from
Nov 22, 2022

Conversation

lsk567
Copy link
Collaborator

@lsk567 lsk567 commented Nov 22, 2022

This PR fixes the Makefile for building the trace tools.

Copy link
Collaborator

@cmnrd cmnrd left a comment

Choose a reason for hiding this comment

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

Shouldn't the trace tools for C rather be located in the reactor-c repository? Or maybe in its own repo? They are only useful for the C target and also only really interesting for us developers (at least at the moment). Also it seems pretty weird to me, to have include paths pointing into our LF source tree. Even if we decide to keep it here, the build should be tested as part of CI so that it does not break again.

Note that the tools for C++ tracing are currently also located in the reactor-cpp repo. Also

@cmnrd cmnrd added c Related to C target bugfix labels Nov 22, 2022
Copy link
Collaborator

@edwardalee edwardalee left a comment

Choose a reason for hiding this comment

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

LGTM. I agree that this should be in reactor-c, but I suggest that be done as a separate PR.

@cmnrd cmnrd merged commit 66a4e45 into master Nov 22, 2022
@cmnrd cmnrd deleted the fix-tracing branch November 22, 2022 17:56
@lhstrh lhstrh changed the title Fix tracing Tracing tools Makefile fixed Jan 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix c Related to C target
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants