Skip to content
This repository has been archived by the owner on Mar 21, 2024. It is now read-only.

Fix contributing guide link to cmake options #1639

Merged
merged 1 commit into from
Mar 22, 2022
Merged

Conversation

jrhemstad
Copy link
Collaborator

No description provided.

@jrhemstad jrhemstad requested a review from brycelelbach March 21, 2022 14:43
@jrhemstad jrhemstad added the only: docs Documentation changes only. Doesn't need code CI. label Mar 21, 2022
@alliepiper alliepiper added this to the 1.17.0 milestone Mar 21, 2022
@@ -86,7 +86,7 @@ cmake --build . -j <num jobs> # invokes make (or ninja, etc)
ctest
```

See [CMake Options](./setup/cmake_options.md) for details on customizing the build. To
See [CMake Options](../setup/cmake_options.md) for details on customizing the build. To
Copy link
Collaborator

@alliepiper alliepiper Mar 21, 2022

Choose a reason for hiding this comment

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

This will still be a broken link on the new doc site: https://nvidia.github.io/thrust/contributing/submitting_a_pr.html

The link is generated to point to .../cmake_options.md, but the link needs to point at a .../cmake_options.html: https://nvidia.github.io/thrust/setup/cmake_options.html

@brycelelbach Is there some doxybook magic we can use to convert the file extension in the link so that both the markdown and doxy versions will work?

Copy link
Collaborator Author

@jrhemstad jrhemstad Mar 21, 2022

Choose a reason for hiding this comment

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

I'm not following. The current formulation ./setup/cmake_options.md points to contributing/setup/cmake_options.md (which doesn't exist).

Wouldn't changing the relative path to ../setup to reference the parent directory correct it?

Copy link
Collaborator

Choose a reason for hiding this comment

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

There is already something in Doxybook/Jekyll that should do this (rewrite .md to .html). It's one of the Jekyll modules, I think?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@brycelelbach whatever is supposed to be rewriting .md to .html in URLs is broken, because it's not happening, and the link to CMake Options generates a 404 on thrust.github.io.

Copy link
Collaborator

@alliepiper alliepiper left a comment

Choose a reason for hiding this comment

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

Let's just merge it and see if the extension is fixed in the docs.

@alliepiper alliepiper merged commit eb3d416 into main Mar 22, 2022
@alliepiper alliepiper deleted the jrhemstad-patch-1 branch March 22, 2022 19:11
@ericniebler
Copy link
Collaborator

ericniebler commented Jun 14, 2022

Let's just merge it and see if the extension is fixed in the docs.

Narrator: It wasn't.

EDIT: NVIDIA/cccl#813

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
only: docs Documentation changes only. Doesn't need code CI.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants