Skip to content

Conversation

dschaefer2
Copy link
Member

There currently are restrictions on the ability for build plugins to generate C header, modulemap, and APInote files. This removes those restrictions when using Swift Build and adds the output directory for the plugin to the HEADER_SEARCH_PATH for the target.

There currently are restrictions on the ability for build plugins to generate C header, modulemap, and APInote files. This removes those restrictions when using Swift Build and adds the output directory for the plugin to the HEADER_SEARCH_PATH for the target.
@dschaefer2
Copy link
Member Author

@swift-ci please test


// With Swift Build on the horizon, we won't add support for generated headers, modulemaps, and apinotes here
for absPath in pluginTargetFiles.headers {
observabilityScope.emit(warning: "Module maps generated by plugins are not supported at this time: \(absPath)")
Copy link
Contributor

Choose a reason for hiding this comment

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

This should say headers

observabilityScope.emit(warning: "Module maps generated by plugins are not supported at this time: \(absPath)")
}
for absPath in pluginTargetFiles.moduleMaps {
observabilityScope.emit(warning: "API Notes generated by plugins are not supported at this time: \(absPath)")
Copy link
Contributor

Choose a reason for hiding this comment

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

This should say module maps

if absPath.extension == "swift" || toolsVersion >= .v6_3 {
files.sources.insert(absPath)
} else {
observabilityScope.emit(warning: "Only Swift is supported for generated plugin source files at this time: \(absPath)")
Copy link
Contributor

Choose a reason for hiding this comment

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

"at this time" is weird now; this should instead mention that it's not supported in the current tools version

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, it's a weird statement. I guess Boris was just trying to give users hope :).

@dschaefer2
Copy link
Member Author

@swift-ci please test

Copy link
Contributor

@bkhouri bkhouri left a comment

Choose a reason for hiding this comment

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

I known this change is still in draft, but can we please add tests, trying to follow the test pyramid. Ie: more small (ie: unit test) and less large tests (ie: end-to-end)

@dschaefer2
Copy link
Member Author

I known this change is still in draft, but can we please add tests, trying to follow the test pyramid. Ie: more small (ie: unit test) and less large tests (ie: end-to-end)

I always do ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

3 participants