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

Add missing CFLAGS, CPPFLAGS and LDFLAGS for some examples/tests #462

Merged
merged 1 commit into from Jul 1, 2019
Merged

Add missing CFLAGS, CPPFLAGS and LDFLAGS for some examples/tests #462

merged 1 commit into from Jul 1, 2019

Conversation

wiene
Copy link
Contributor

@wiene wiene commented Jun 16, 2019

This patch ensures that CFLAGS, CPPFLAGS and LDFLAGS settings are
respected when compiling/linking (lib)sotest, pivot_root and userns.

I was kindly made aware of this issue by the blhc tool.

This patch ensures that CFLAGS, CPPFLAGS and LDFLAGS settings are
respected when compiling/linking (lib)sotest, pivot_root and userns.
@reidpr reidpr added the bug label Jun 17, 2019
@reidpr reidpr added this to the next milestone Jun 17, 2019
Copy link
Collaborator

@reidpr reidpr left a comment

Choose a reason for hiding this comment

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

Thanks Peter!

I'd also be open to a PR adding blhc to the test suite. Do you think that's a good idea?

@@ -10,4 +10,4 @@ clean:
$(BINS): Makefile

%: %.c
gcc $(CFLAGS) $< -o $@
gcc $(CFLAGS) $(CPPFLAGS) $(LDFLAGS) $< -o $@
Copy link
Collaborator

Choose a reason for hiding this comment

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

These are C programs, so do we really want $CPPFLAGS?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since the C compiler uses the C preprocessor, I think that it makes sense to also include C preprocessor flags.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oops, I'm sorry. I thought CPPFLAGS was for C++.

@wiene
Copy link
Contributor Author

wiene commented Jun 18, 2019

I'd also be open to a PR adding blhc to the test suite. Do you think that's a good idea?

So far I have no experience with using blhc outside a Debian environment where it is part of the CI machinery. Citing blhc's README file (for version 0.09):

At the moment it works only on Debian and derivatives but it should be easily
extendable to other systems as well.

[...]

The build log must contain the following line which marks the beginning of the
real compile process (output of dpkg-buildpackage):

dpkg-buildpackage: ...

If it's not present no compiler commands are detected. In case you don't use
dpkp-buildpackage but still want to check a build log, adding it as first line
should work fine.

So in summary, I feel uncertain how easy or difficult it is to integrate it in the Charliecloud test suite.

@reidpr
Copy link
Collaborator

reidpr commented Jun 18, 2019

So in summary, I feel uncertain how easy or difficult it is to integrate it in the Charliecloud test suite.

OK. This part of the code doesn't change too much, and it sounds like you would catch it eventually in the Debian package, so that seems like a low priority to add it upstream?

@reidpr reidpr self-requested a review June 18, 2019 15:03
@reidpr reidpr modified the milestones: next, 0.9.11 Jun 18, 2019
@reidpr reidpr merged commit f0b867b into hpc:master Jul 1, 2019
@wiene wiene deleted the add-flags branch July 17, 2023 19:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants