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.

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

feat: add string_literal to module identifier allow list #1319

Merged
merged 11 commits into from
Nov 5, 2024
Merged

feat: add string_literal to module identifier allow list #1319

merged 11 commits into from
Nov 5, 2024
+30 −8

Conversation

lucasmarcal-faire
Copy link
Contributor

@lucasmarcal-faire lucasmarcal-faire commented Oct 30, 2024 •
edited
Loading

Add string_literal to module identifier allow list.
This case seems to be pretty rare, and besides the fact that it's not mentioned on ModuleMap specs, Clang actually accepts that.

It's rare, but you can find examples by searching for module " path:/(^|\/)*\.modulemap$/ on Github

The motivation behind creating this PR is to add support to the swift-llbuild Package build that fails while resolving this modulemap

This PR alone doesn't fix swift-llbuild build. Some follow-up PRs will be created to add SPM include folder handling

@lucasmarcal-faire lucasmarcal-faire changed the title [WIP] Add string_literal to module identifier allow list to support umbrella header declaration Add string_literal to module identifier allow list to support umbrella header declaration Oct 30, 2024
@lucasmarcal-faire lucasmarcal-faire changed the title Add string_literal to module identifier allow list to support umbrella header declaration [WIP] Add string_literal to module identifier allow list to support umbrella header declaration Oct 30, 2024
@cgrindel cgrindel marked this pull request as draft October 31, 2024 00:41
@cgrindel
Copy link
Owner

I converted this to a draft while you are still working on it. When you would like a review, feel free to transition it out of the draft state.

@lucasmarcal-faire
remove header reference from comment
e517115
@lucasmarcal-faire
Merge branch 'lucas-marcal/allow_string_literal_modulemap_header_decl…
f849202 
…aration' of github.com:lucasmarcal-faire/rules_swift_package_manager into lucas-marcal/allow_string_literal_modulemap_header_declaration
@lucasmarcal-faire lucasmarcal-faire changed the title [WIP] Add string_literal to module identifier allow list to support umbrella header declaration [WIP] Add string_literal to module identifier allow list Oct 31, 2024
@lucasmarcal-faire lucasmarcal-faire changed the title [WIP] Add string_literal to module identifier allow list Add string_literal to module identifier allow list Oct 31, 2024
@lucasmarcal-faire lucasmarcal-faire marked this pull request as ready for review October 31, 2024 14:50
cgrindel
cgrindel approved these changes Oct 31, 2024
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.

Please incorporate my proposed update to the comment. That should provide enough breadcrumbs to understand why we support string literals.

swiftpkg/internal/modulemap_parser/collect_module.bzl Show resolved Hide resolved
@cgrindel cgrindel changed the title Add string_literal to module identifier allow list feat: add string_literal to module identifier allow list Oct 31, 2024
@lucasmarcal-faire @cgrindel
Update swiftpkg/internal/modulemap_parser/collect_module.bzl
cb7585e 
Co-authored-by: Chuck Grindel <chuck.grindel@gmail.com>
@cgrindel
Copy link
Owner

cgrindel commented Nov 1, 2024

@Mergifyio queue

@mergify Mergify
Copy link
Contributor

mergify bot commented Nov 1, 2024 •
edited
Loading

queue

🟠 Waiting for conditions to match

  • any of: [🔀 queue conditions]
    • all of: [📌 queue conditions of queue default]
      • any of: [🛡 GitHub branch protection]
        • check-neutral = all_ci_tests
        • check-skipped = all_ci_tests
        • check-success = all_ci_tests
      • any of: [🛡 GitHub branch protection]
        • check-neutral = conventional_commit_check
        • check-skipped = conventional_commit_check
        • check-success = conventional_commit_check
      • #approved-reviews-by >= 1 [🛡 GitHub branch protection]
      • #changes-requested-reviews-by = 0 [🛡 GitHub branch protection]
      • #review-threads-unresolved = 0 [🛡 GitHub branch protection]
  • -closed [📌 queue requirement]
  • -conflict [📌 queue requirement]
  • -draft [📌 queue requirement]
  • any of: [📌 queue -> configuration change requirements]
    • -mergify-configuration-changed
    • check-success = Configuration changed

@lucasmarcal-faire
Copy link
Contributor Author

I guess I have to fix some tests, let me push a commit with that

@lucasmarcal-faire
Copy link
Contributor Author

@cgrindel Failing tests are now fixed.

@cgrindel
Copy link
Owner

cgrindel commented Nov 5, 2024

It looks like you need to run bazel run //:tidy.

@lucasmarcal-faire
Copy link
Contributor Author

@cgrindel done!

@cgrindel
Copy link
Owner

cgrindel commented Nov 5, 2024

@Mergifyio queue

@mergify Mergify
Copy link
Contributor

mergify bot commented Nov 5, 2024 •
edited
Loading

queue

✅ The pull request has been merged automatically

The pull request has been merged automatically at 8d4e713

@mergify mergify bot merged commit 8d4e713 into cgrindel:main Nov 5, 2024
33 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@adincebic adincebic adincebic approved these changes

@cgrindel cgrindel Awaiting requested review from cgrindel

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

Successfully merging this pull request may close these issues.
3 participants
@lucasmarcal-faire @cgrindel @adincebic