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

Add executorFactory option to package manifest. #8443

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

al45tair
Copy link
Contributor

@al45tair al45tair commented Apr 2, 2025

This lets you specify which executor factory to use in the Package.swift.

rdar://148430450

This lets you specify which executor factory to use in the `Package.swift`.

rdar://148430450
@al45tair
Copy link
Contributor Author

al45tair commented Apr 2, 2025

@swift-ci Please smoke test

Copy link
Contributor

@MaxDesiatov MaxDesiatov left a comment

Choose a reason for hiding this comment

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

What's the corresponding Swift Evolution proposal/pitch for this? IIUC changes to PackageDescription public API are usually required to be pitched before merging.

@dschaefer2
Copy link
Member

Yup, a quick pitch should be fine just to let the community know. Also some tests :).

@MaxDesiatov MaxDesiatov added needs tests This change needs test coverage package manifests changes to package manifest APIs labels Apr 2, 2025
@al45tair
Copy link
Contributor Author

al45tair commented Apr 2, 2025

What's the corresponding Swift Evolution proposal/pitch for this?

Pitch is here:
https://forums.swift.org/t/pitch-2-custom-main-and-global-executors/78437

Also some tests :).

Indeed, I'd like to add some tests, though I find the SwiftPM repo hard to navigate so it would be useful if someone could point me in the right place(s) for that.

@MaxDesiatov
Copy link
Contributor

You could modify the existing testBuildSettings in BuildPlantTests.swift, although it probably be better to create a separate test function so that it can run in parallel when these are migrated to Swift Testing.

@jakepetroules
Copy link
Contributor

@al45tair Please add a new build setting in Swift Build for this as well. When using the Swift Build build engine backend we're not going to be using OTHER_SWIFT_FLAGS to pass down these sort of things.

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.

chore (blocking): Would it be possible to add an automated tests that verify, at a minimum:

  • A valid value is passed for the executor factory
  • an invalid value is passed to the executor Factory
  • the executor factory is not specified

This will ensure we the behaviour works as intended and that it does not regress

@al45tair
Copy link
Contributor Author

al45tair commented Apr 3, 2025

Would it be possible to add an automated tests that verify, at a minimum:

  • A valid value is passed for the executor factory
  • an invalid value is passed to the executor Factory
  • the executor factory is not specified

This will ensure we the behaviour works as intended and that it does not regress

It doesn't make sense to test all of that here. Tests of compiler options belong in the compiler tests. SwiftPM might reasonably test that the option is passed through to the compiler — it shouldn't be testing compiler behaviour.

@al45tair
Copy link
Contributor Author

al45tair commented Apr 3, 2025

(I'll add to that that I think it would make a lot of sense if SwiftPM and Swift Build picked up options automatically from the .td files in the main Swift repo; there could be a flag that causes the option to be added to the swiftSettings automatically. For a lot of compiler options, including this one, that would be more than sufficient and would avoid people working on compiler features having to open two separate PRs and write separate sets of tests for each of these things. swift-driver already does pretty much this, though it isn't wholly automated and you need to open a PR against it to get an option added.)

Add some tests of the executorFactory setting.

rdar://148430450
@al45tair al45tair requested review from MaxDesiatov and bkhouri April 7, 2025 16:38
@al45tair
Copy link
Contributor Author

al45tair commented Apr 7, 2025

@swift-ci Please test

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs tests This change needs test coverage package manifests changes to package manifest APIs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants