Skip to content

fix: add .def files to textual_hdrs of clang targets #1600

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 10 commits into
base: main
Choose a base branch
from
Open

fix: add .def files to textual_hdrs of clang targets #1600

wants to merge 10 commits into from

Conversation

adincebic
Copy link
Contributor

@adincebic adincebic commented May 4, 2025 •
edited
Loading

Some packages like KSCrash rely on .def files as textual headers.
I first noticed this while investigating #1279

@adincebic adincebic changed the title Add .def files to textual_hdrs of cc_library Add .def files to textual_hdrs of clang targets May 4, 2025
@adincebic adincebic changed the title Add .def files to textual_hdrs of clang targets fix: add .def files to textual_hdrs of clang targets May 4, 2025
@adincebic adincebic force-pushed the adin/support-def-files branch from 838770a to 21cfd24 Compare May 5, 2025 06:12
@adincebic adincebic mentioned this pull request May 7, 2025
Open
cgrindel
cgrindel reviewed May 7, 2025
View reviewed changes
Copy link
Owner

@cgrindel cgrindel left a comment

Choose a reason for hiding this comment

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

I would prefer that we have a test and/or example that exercises this functionality. In an example, could we patch the KSCrash Package.swift to link the zlib library?

@adincebic
Copy link
Contributor Author

I would prefer that we have a test and/or example that exercises this functionality. In an example, could we patch the KSCrash Package.swift to link the zlib library?

Makes sense, gonna do that later today.

@adincebic adincebic force-pushed the adin/support-def-files branch from 21cfd24 to ea2f268 Compare May 8, 2025 10:15
@adincebic
Copy link
Contributor Author

@cgrindel what should I do to make CI green? Looks like I am missing something obvious

@cgrindel
Copy link
Owner

cgrindel commented May 8, 2025

@cgrindel what should I do to make CI green? Looks like I am missing something obvious

It looks like there was a .build/... directory in your source tree when you ran //:tidy. It added the contents of that directory which included a sub-directory that had a space in the name. I cleared the deleted packages values in .bazelrc and ran //:tidy, again.

cgrindel
cgrindel reviewed May 8, 2025
View reviewed changes
Copy link
Owner

@cgrindel cgrindel left a comment

Choose a reason for hiding this comment

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

Overall, looks good. A few comments and we should be be good to go.

examples/kscrash_example/MODULE.bazel Outdated
"swiftpkg_kscrash",
)

# We patch KSCrash because it implicitly links against zlib and that information isn't available to rules_swift_package_manager
Copy link
Owner

Choose a reason for hiding this comment

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

Could we wrap this comment to 80 characters?

examples/kscrash_example/Sources/MyApp/MyApp.swift Outdated
Copy link
Owner

Choose a reason for hiding this comment

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

Can you convert the tabs to four spaces?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Those are spaces already. But I'll run swiftformat over this file.

examples/kscrash_example/do_test Outdated
Comment on lines 11 to 15
# GH249: Do not update build files until the Gazelle extension supports the
# shape of this example.
# # Generate Swift external deps and update build files
# bazel run //:tidy
# "${bazel}" run //:swift_update_pkgs
Copy link
Owner

Choose a reason for hiding this comment

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

Is this true? Does the gazelle plugin not work properly for this example?

examples/kscrash_example/third_party/BUILD.bazel Outdated
Copy link
Owner

Choose a reason for hiding this comment

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

Could we move this file to examples/kscrash_example/third_party/KSCrash? We will then need to update the reference to the patch in the MODULE.bazel.

@adincebic adincebic force-pushed the adin/support-def-files branch from 1227123 to fc9c776 Compare May 8, 2025 16:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cgrindel cgrindel cgrindel left review comments

@brentleyjones brentleyjones Awaiting requested review from brentleyjones brentleyjones is a code owner

@jpsim jpsim Awaiting requested review from jpsim jpsim is a code owner

@luispadron luispadron Awaiting requested review from luispadron luispadron is a code owner

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@adincebic @cgrindel