-
Notifications
You must be signed in to change notification settings - Fork 43
Fix deduplication of -enable-experimental-feature #1044
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this 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?
| 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) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 ifdedup = Falsewas passed, it will return the list without deduping. - Move adding
-enable-experimental-featureafter runningto_starlark, so that it skips the deduplication. This allows us to reuseto_starlarkas it is now. Experimental features will need to be placed in a different list, so it doesn't affect the othercoptsfrom unsafe flags.
There was a problem hiding this comment.
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?)
|
@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. |
|
Alternatively, we could convert it to a Draft PR. |
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.paramsfile that is used to set the build arguments, where-enable-experimental-featurewas deduplicated. The deduplication happens duringbzl_selects.to_starlarkas it operates on sets, instead of lists.Changes:
bzl_selects.unwrap_listto operate on lists instead of setsunwrap_list, as it shouldn't be treated as a set.