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

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

Open
heckj opened this issue Feb 24, 2023 · 7 comments
Assignees
Labels
enhancement Improvements or enhancements to existing functionality good first issue Good for newcomers

Comments

@heckj
Copy link
Member

heckj commented Feb 24, 2023

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 heckj added the enhancement Improvements or enhancements to existing functionality label Feb 24, 2023
heckj added a commit to heckj/swift-docc that referenced this issue Feb 24, 2023
- related to swiftlang#490, updates the
  documentation to identify that --checkout-path requires an absolute
  oath, not a relative path.
franklinsch pushed a commit to heckj/swift-docc that referenced this issue Feb 27, 2023
- related to swiftlang#490, updates the
  documentation to identify that --checkout-path requires an absolute
  oath, not a relative path.
franklinsch pushed a commit that referenced this issue Feb 27, 2023
* documentation update to clarify CLI option

- related to #490, updates the
  documentation to identify that --checkout-path requires an absolute
  oath, not a relative path.

* Update Sources/docc/DocCDocumentation.docc/distributing-documentation-to-other-developers.md
@heckj
Copy link
Member Author

heckj commented Apr 24, 2023

/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

@d-ronnqvist d-ronnqvist added the good first issue Good for newcomers label Dec 21, 2023
@Anthony-Eid
Copy link
Contributor

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

@Anthony-Eid
Copy link
Contributor

Anthony-Eid commented May 24, 2024

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
Copy link
Member Author

heckj commented May 24, 2024

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

@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 #934 to resolve the failure.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Improvements or enhancements to existing functionality good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

4 participants