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 common compilation flags to tests [BUILD-448] #25

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

krisukox
Copy link
Contributor

@krisukox krisukox commented Dec 13, 2022

This PR adds common compilation flags to swift_cc_test_library and adds common_c_opts flag to swift_cc_test which adds common compilation flags.

This PR sets the linkstatic flag to True in swift_cc_test.

Tested here:

jungleraptor
jungleraptor previously approved these changes Dec 13, 2022
Copy link
Contributor

@jungleraptor jungleraptor left a comment

Choose a reason for hiding this comment

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

On second thought lets go ahead and merge this.

But I think our goal once we get everything building with these compile flags should be to remove the bool flag.

"""Wraps cc_test to enforce Swift testing conventions.

Args:
name: A unique name for this rule.
type: Specifies whether the test is a unit or integration test.
common_c_opts: Bool flag that indicates if common compilation flags will be added. Default False.
Copy link
Contributor

Choose a reason for hiding this comment

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

Were there things that failed to build unless this was an optional parameter?

Guessing in a lot of cases we need to remove certain flags via nocopts.

kwargs["name"] = name
kwargs["tags"] = (kwargs["tags"] if "tags" in kwargs else []) + [type]
kwargs["linkstatic"] = (kwargs["linkstatic"] if "linkstatic" in kwargs else True)
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the reason for making this the default?

@jungleraptor jungleraptor dismissed their stale review December 13, 2022 23:46

Changed mind again

Copy link
Contributor

@jungleraptor jungleraptor left a comment

Choose a reason for hiding this comment

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

I went back and forth but lets discuss a little more in our meeting tomorrow. Basically I don't want to have that bool flag become a permanent part of the API and am a little concerned that it might become difficult to remove it if we merge these changes as is.

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.

None yet

2 participants