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

cc: split up rules for building c and c++ code [BUILD-560] #52

Merged
merged 7 commits into from
Mar 16, 2023

Conversation

jungleraptor
Copy link
Contributor

@jungleraptor jungleraptor commented Mar 15, 2023

Splits up our swift_cc* wrappers into two sets of functions for building c and c++ code separately

Design Notes

We use the swift wrappers to enforce that (among other options), we set the following flags consistently across our codebase:

  • -fno-exceptions
  • -fno-rtti
  • -std=c++14
  • -std=c99

Unfortunately bazel cc* rules don't support distinguishing between options for the c compiler and options for the c++ compiler:

cc_library(
    name = "gnss_converters",
    srcs = [
        "src/common.c", # c file
        "src/time_truth.cc", # c++ file
    ],
    hdrs = [
       ...
    ],
    copts = [ # copts applies to both .c and .cc files
       "-fno-exceptions", # Warning when applied to c file
       "-Werror", # Warning is promoted to an error
    ]
)

The solution proposed here is to split up building c and c++ code into two functions so the problematic flags can be applied separately.

Usage

Examples, see related PR's for actual implementations:

swift_c_library(
    name = "time_truth",
    srcs = [
        "src/time_truth.c",
    ],
    extensions = True, # enables GNU extensions; -std=gnu
    standard = 11, # sets the c11 standard
    ...
)

swift_cc_library(
    name = "time_truth_v2",
    exceptions = True, # build with -fexceptions
    rtti = True, # build with -frtti
    standard = 17, # build with c++17 standard
    srcs = [
        "src/time_truth_v2.cc",
    ],
    ...
)

swift_c_library(
    name = "gnss_converters",
    deps = [
        ":time_truth",
        ":time_truth_v2",
    ],
)

Also adds flags for enabling exceptions and rtti globally:

bazel build --@rules_swiftnav//cc:enable_exceptions=true //...
bazel build --@rules_swiftnav//cc:enable_rtti=true //...

Also adds a flag for overriding the default c++ standard:

bazel build --@rules_swiftnav//:cc:cxx_standard=17 //...

Testing

New features were tested in the following PR's:

Related PR's:

This PR cannot be merged in standalone without breaking a whole bunch of builds.
We'll need to first merge empty stub functions across the codebase, then come back and merge this one:

@jungleraptor jungleraptor changed the title cc: implement c only rules [BUILD-560] cc: split up rules for building c and c++ code [BUILD-560] Mar 15, 2023
@jungleraptor jungleraptor requested a review from a team March 15, 2023 23:31
@jungleraptor jungleraptor marked this pull request as ready for review March 15, 2023 23:31
@jungleraptor
Copy link
Contributor Author

jungleraptor commented Mar 15, 2023

cc @woodfell @RReichert thought it'd be good to tag you two for review. This essentially wraps up the API for the swift_cc rules wrappers.

@RReichert
Copy link
Contributor

@isaactorz the one area that I see that might have issues with this is gnss_converters where were have C/C++ code in a single library, maybe some of the tests as well might face some issues.

@jungleraptor
Copy link
Contributor Author

@isaactorz the one area that I see that might have issues with this is gnss_converters where were have C/C++ code in a single library, maybe some of the tests as well might face some issues.

Check out this PR where I've tested: https://github.com/swift-nav/gnss-converters-private/pull/1468

But basically getting gnss-converters built correctly is what this PR addresses, since it's pretty much the only place in the codebase where we've been mixing languages.

Copy link
Contributor

@krisukox krisukox left a comment

Choose a reason for hiding this comment

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

I tested it locally with gnss_converters, and it worked well. LGTM.

@jungleraptor jungleraptor merged commit f631fd6 into main Mar 16, 2023
@jungleraptor jungleraptor deleted the isaac/add-c-only-rules branch March 16, 2023 23:28
jungleraptor added a commit that referenced this pull request Mar 21, 2023
jungleraptor added a commit that referenced this pull request Mar 22, 2023
)" (#55)

This reverts commit f631fd6.

This was merged too early. Disabling until ready.
jungleraptor added a commit that referenced this pull request Mar 23, 2023
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.

3 participants