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

Fix deduplication of -enable-experimental-feature #1044

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

Dreksh
Copy link

@Dreksh Dreksh commented Apr 25, 2024

When using a Swift package that contains multiple experimental features (i.e. https://github.com/sersoft-gmbh/swift-smtp ), the error that is returned shows treats one of the features as an input file.

This is due to the generated parameters within the *.swiftmodule-0.params file that is used to set the build arguments, where -enable-experimental-feature was deduplicated. The deduplication happens during bzl_selects.to_starlark as it operates on sets, instead of lists.

Changes:

  • Introduced bzl_selects.unwrap_list to operate on lists instead of sets
  • Changed the copts to rely on unwrap_list, as it shouldn't be treated as a set.

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.

Thank you for the contribution! Please see my comment asking whether we can handle this inside to_starlark(). Also, could we add https://github.com/sersoft-gmbh/swift-smtp to one of the examples so that we reproduce the issue and confirm that it stays fixed?

@@ -106,7 +106,7 @@ def _swift_target_build_file(pkg_ctx, target):
if len(defines) > 0:
attrs["defines"] = bzl_selects.to_starlark(defines)
if len(copts) > 0:
attrs["copts"] = bzl_selects.to_starlark(copts)
attrs["copts"] = bzl_selects.unwrap_list(copts)
Copy link
Owner

Choose a reason for hiding this comment

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

Is there any way to detect this in bzl_selects.to_starlark()? It is not obvious to me why copts would get this special handling.

Copy link
Author

Choose a reason for hiding this comment

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

The reason is that we add a string repeatedly:

copts.append("-enable-experimental-feature")

to_starlark relies on using sets within its processing, which will deduplicate the string that's added.

I'm not familiar with the repository, so I decided to add a new function to reduce the chances that I'll affect other places that use to_starlark.

Possible Alternatives:

  • Add a parameter like dedup = True, and only perform deduplication after the list has been created, so that if dedup = False was passed, it will return the list without deduping.
  • Move adding -enable-experimental-feature after running to_starlark, so that it skips the deduplication. This allows us to reuse to_starlark as it is now. Experimental features will need to be placed in a different list, so it doesn't affect the other copts from unsafe flags.

Copy link
Author

Choose a reason for hiding this comment

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

I tried doing -enable-experimental-feature after running to_starlark, but didn't realise that it returns a structure instead of a list. So this method won't work, as to_starlark will have copts in an internal structure. (I did not dig into whether it'll be easy to append values into that list).

I'll give adding the dedup parameter a try (maybe renamed to preserve_list?)

@cgrindel
Copy link
Owner

@Dreksh Were you going to pick this up again? If not, I am inclined to close this. We can always open a new PR when/if you want to continue.

@cgrindel
Copy link
Owner

Alternatively, we could convert it to a Draft PR.

@cgrindel cgrindel marked this pull request as draft December 7, 2024 21:46
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