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

Import build flags from environment (a la Chisel2). #245

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

ucbjrl
Copy link
Contributor

@ucbjrl ucbjrl commented May 3, 2019

Import CC, CXX, CCFLAGS, CXXFLAGS, CPPFLAGS, LDFLAGS and incorporate them in the flags used to build C++ simulations.

Although similar effects can be achieved via the --more-vcs-flags, and --more-vcs-c-flags command line options, it's difficult to make these available to the internal ScalaTest suites.

@ucbjrl ucbjrl requested a review from a team as a code owner May 3, 2019 22:02
Copy link
Contributor

@chick chick left a comment

Choose a reason for hiding this comment

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

I think this looks ok, though I don't have the machinery to test this out.
Could there be a simple test, for instance, something that sets an env var that
calls a script in the test suite that shows arguments are picked up can run?
One minor quibble about a new line.
I was also wondering if there should be some output when environment variables are
overriding default values. Sometimes left-over env vars floating around can cause
weird behavior and it's difficult to know what's causing it.

val CXXFLAGS = envOrElse("CXXFLAGS", "")
val CPPFLAGS = envOrElse("CPPFLAGS", "")
val LDFLAGS = envOrElse("LDFLAGS", "")
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing newline?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch. Will fix.

@chick
Copy link
Contributor

chick commented May 6, 2019

If there's some urgency, maybe my comments should be turned into issues for testers2 so that
it can be a bit more robust

@ucbjrl
Copy link
Contributor Author

ucbjrl commented May 6, 2019

I think this does need to be discussed. I like the idea of issuing some kind of notification when environment variables are used.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants