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

Begin including SPI documentation in local builds, and fix all newly-revealed DocC issues #295

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

stmontgomery
Copy link
Contributor

Begin including documentation for @_spi declarations in local development (non-distribution) builds, and fix all newly-revealed DocC issues in those SPI declarations.

I also took this opportunity to simplify the mechanism for conditionalizing certain package settings for distribution builds, and fix a few other documentation omissions and inconsistencies I noticed.

Checklist:

  • Code and documentation should follow the style of the Style Guide.
  • If public symbols are renamed or modified, DocC references should be updated.

Resolves rdar://118416838

@stmontgomery stmontgomery added the documentation Improvements or additions to documentation label Mar 16, 2024
@stmontgomery stmontgomery self-assigned this Mar 16, 2024
@stmontgomery
Copy link
Contributor Author

@swift-ci please test

// Testing module. This target's module is never distributed to users,
// but as an additional guard against accidental misuse, this specifies
// the unit test target as the only allowable client.
.unsafeFlags(["-Xfrontend", "-allowable-client", "-Xfrontend", "TestingMacrosTests"]),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Even though I still like the idea of this setting, in practice it is not giving us any benefit or protection since currently it must be commented-out for distribution builds (which are the usage scenario it was meant for). Having it here causes us an additional manual step each time we cut a release, and there's little practical value in conditionalizing it to only non-distribution builds IMO.

/// predates the introduction of that type.
/// }
//
// - Bug: The associated value of this enumeration case should be an
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I confirmed that the rendered DocC still shows the documentation block comments above, despite having 2-slash // comments in between.

@grynspan
Copy link
Contributor

Please hold on to this PR.

/// Non-distribution (i.e. development-only) builds may enable things
/// like stricter compiler features which are unsupported or inappropriate for
/// client builds.
let isBuildingForDistribution = false
Copy link
Contributor

Choose a reason for hiding this comment

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

What if instead we checked for an environment variable and built SPI when it's set?

let buildSPIDocumentation: Bool = {
  if let boolValue = Context.environment["SWT_SPI_DOCUMENTATION_ENABLED"].flatMap(Bool.init) {
    return boolValue
  } else if let intValue = Context.environment["SWT_SPI_DOCUMENTATION_ENABLED"].flatMap(UInt64.init) {
    return intValue != 0
  }
  return false // disabled by default
}()

(That's a slightly simplified version of what our own Environment type does.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could do this, although a workflow I commonly use for viewing docs is the "Product > Build Documentation" command in recent versions of Xcode, and I was hoping for that to get this flag too. I don't know of a way to apply an env var in that scenario (when using the GUI Xcode app, not its xcodebuild CLI).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The constant I added here would still be needed, or at least it would still be helpful, even if we conditionalized the SPI docs flag based on an env var, because the constant is what (now) decides whether all the unsafeFlags get included

Copy link
Contributor

Choose a reason for hiding this comment

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

Honestly we can also just drop the unsafe flags outright.

Copy link
Contributor

Choose a reason for hiding this comment

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

although a workflow I commonly use for viewing docs is the "Product > Build Documentation" command in recent versions of Xcode

We can ask the DocC folks if they can help with this.

Copy link
Contributor

@Kyle-Ye Kyle-Ye Jul 1, 2024

Choose a reason for hiding this comment

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

We could do this, although a workflow I commonly use for viewing docs is the "Product > Build Documentation" command in recent versions of Xcode, and I was hoping for that to get this flag too. I don't know of a way to apply an env var in that scenario (when using the GUI Xcode app, not its xcodebuild CLI).

+1 for some similar scenario I experienced. And I believe it is a Xcode/SwiftPM issue instead of a DocC issue.

Xcode GUI only gives us the ability to set env value for executable target when they are running. I could not find a easy solution to setup the env value I use in my Package.swift file.

Currently I just use a small macOS app I wrote to trigger export XX=YY && open ./ -a Xocde for me in Finder.

Otherwise we'll have to quit Xcode and manually export the env value in shell and then open Xcode in the shell to make Package.swift access the env we set.

image

As a person who maintains some Swift packages and likes to add environment variable condition config in package, it is really troublesome at present if we do not use any other 3rd party tools.

Copy link
Contributor

@grynspan grynspan left a comment

Choose a reason for hiding this comment

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

I think we can avoid having to manually toggle isBuildingForDistribution. See my comment above about an environment variable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants