swiftlang / swift-docc

Documentation compiler that produces rich API reference documentation and interactive tutorials for your Swift framework or package.
https://swift.org/documentation/docc
Apache License 2.0
1.19k stars 127 forks source link

Support a relative path for the options to "link to source" for the --target-path CLI option #490

Open heckj opened 1 year ago

heckj commented 1 year ago

Feature Name

support relative paths for --target-path

Description

I started trying to use the feature for including links to hosted online source, as described in Distributing Documentation to Other Developers.

The --target-path option currently requires an absolute path to the location to reference the source, and it would tremendously more convenient if this were provided in the form of a relative path.

Per a thread in swift forums, Franklin made a reference to a likely location for the possible enhancement.

Motivation

Allowing a relative path makes any CI/scripted work significantly easier for generating documentation, as it more easily divorces the location from the machine. It's possible to work around this in a shell script (something like "$(dirname $(dirname $(filepath $0)))", as seen in https://github.com/apple/swift-markdown/blob/main/bin/update-gh-pages-documentation-site), but that's not common knowledge or easily found.

Importance

Entirely an ease-of-use scenario

Alternatives Considered

No response

heckj commented 1 year ago

/cc @ethan-kusters @franklinsch @QuietMisdreavus - the symbol graph extension issue that I mentioned, where extraction from a local binary target resulted in no symbols being found

Anthony-Eid commented 9 months ago

Hi, I'm interested in working on this issue. Could I please be assigned it?

Anthony-Eid commented 5 months ago

For clarification is the goal of this issue to add support for using relative paths with the --checkout-path argument, or creating a new argument --target-path that uses relative paths instead of absolute paths?

Edit:

I understand that changes should be made to how checkoutPath is initialized in the SourceRepository struct to enable relative paths. However, I am unsure about the exact conditions under which a path should be considered relative versus absolute.

Specifically, should a path be assumed to be absolute only if it begins with a slash ("/")? Likewise, should a path be assumed to be relative if it beings with a period (".")?

If a path doesn't begin with either a period or a comma, should we validate whether the path is valid as an absolute path or a relative path, giving precedence to treating it as an absolute path if both are valid? Otherwise, using whichever path (relative or absolute) is valid.

heckj commented 5 months ago

For my part, I would tend to lean to defaulting to a relative path if there wasn't a clear indicator of an absolute path, which aligns with more of the Unix tools patterns and their general philosophy. So if the path begins with a /, then definitely an absolute reference, otherwise assume a relative path. But that is a point of view that's naive of any implications that are already in place within the existing code.

d-ronnqvist commented 5 months ago

For clarification is the goal of this issue to add support for using relative paths with the --checkout-path argument, or creating a new argument --target-path that uses relative paths instead of absolute paths?

The goal is to use the same command line option for both relative and absolute paths.

However, I am unsure about the exact conditions under which a path should be considered relative versus absolute.

Specifically, should a path be assumed to be absolute only if it begins with a slash ("/")? Likewise, should a path be assumed to be relative if it beings with a period (".")?

I would try to not inspect the raw paths and instead rely on Foundation to determine what's an absolute and a relative file path. You can see that a URL will include the current directory for a relative path string but not for an absolute path string:

print(URL(fileURLWithPath: "/some/absolute/url").absoluteString) 
// "file:///some/absolute/url"
print(URL(fileURLWithPath: "some/relative/url").absoluteString) 
// "file:///path/to/current-directory/some/relative/url"

Since this is a user provided command line option, it could also be good to check that the checkout directory exists using

guard FileManager.default.directoryExists(atPath: theDeveloperProvidedCheckoutPath) else {
    throw ValidationError("...")
}
Anthony-Eid commented 5 months ago

I have all the test cases except testParameterValidationFeatureFlag() passing right now. testParameterValidationFeatureFlag() only passes when it's ran without other tests, otherwise it fails and throws this error message.

testParameterValidationFeatureFlag(): failed: caught error: "CommandError(commandStack: [SwiftDocCUtilities.Docc.Convert], parserError: ArgumentParser.ParserError.noArguments(ArgumentParser.ParserError.userValidationError(Invalid path provided via the 'DOCC_HTML_DIR' environment variable. No directory exists at '/Users/eid/Library/Developer/Xcode/DerivedData/swift-docc-fkdqlopyxcemcsfoqmgdqkwhcole/Build/Products/Debug/ConvertSubcommandTests-testOptionsValidation/7CEDAF49-0F53-4F85-B48D-35F87E748263-32724-00000462E9F9A917'.)))"

Several other test cases within the ConvertSubcommandTests class change/set the DOCC_HTML_DIR environmental variable. So the error is probably caused by other test cases messing with the value of DOCC_HTML_DIR. Is this something that I should be worried about?

The same behavior is persistent when I'm running the unit tests locally against apple:main branch. However, I noticed that the tests are passing when ran in the CICD. So the issue is probably on my end.

QuietMisdreavus commented 5 months ago

@Anthony-Eid I had run into that issue myself for a while and hadn't narrowed it down. It turns out that the failure was when the tests were run sequentially (i.e. swift test as opposed to bin/test or swift test --parallel); i've opened https://github.com/apple/swift-docc/pull/934 to resolve the failure.