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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants