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.

Sign up for GitHub

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

Jump to bottom

cmake: Freshen up docs/INSTALL.cmake #12772

Closed
wants to merge 1 commit into from
Closed

cmake: Freshen up docs/INSTALL.cmake #12772

wants to merge 1 commit into from

Conversation

levitte
Copy link
Contributor

@levitte levitte commented Jan 24, 2024 •
edited

  • Turn docs/INSTALL.cmake into a proper markdown file, docs/INSTALL-CMAKE.md
  • Move things around to divide the description into configuration, building
    and installing sections
  • Mention the more modern cmake options to configure, build and install, but
    also retain the older variants as fallbacks

vszakats
vszakats reviewed Jan 24, 2024
View reviewed changes
docs/INSTALL.cmake Outdated Show resolved Hide resolved
@levitte levitte marked this pull request as draft January 24, 2024 13:36
bagder
bagder reviewed Jan 26, 2024
View reviewed changes
Copy link
Member

@bagder bagder left a comment

Choose a reason for hiding this comment

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

Maybe change this document to use a .md extension so that it will be treated as markdown properly by GitHub, editors etc?

@levitte
Copy link
Contributor Author

levitte commented Jan 26, 2024

Maybe change this document to use a .md extension so that it will be treated as markdown properly by GitHub, editors etc?

Sure. INSTALL.cmake.md, then?

@levitte
Copy link
Contributor Author

levitte commented Jan 26, 2024

Fixup commit added, for separate review. I'll squash it into the first commit later

@levitte levitte force-pushed the freshen-INSTALL.cmake branch 4 times, most recently from 81c003c to d57a721 Compare January 27, 2024 08:55
@github-actions github-actions bot added the CI Continuous Integration label Jan 27, 2024
@levitte
Copy link
Contributor Author

levitte commented Jan 27, 2024

I listened to both criticism (the minimum cmake requirement) and ideas (turn it into a proper markdown file) and took a stab at trying to answer them all. I hope that what I've now pushed does the trick.

@levitte levitte force-pushed the freshen-INSTALL.cmake branch 2 times, most recently from 6bd0d5c to c020003 Compare January 27, 2024 09:42
@levitte levitte marked this pull request as ready for review January 27, 2024 09:43
@levitte
Copy link
Contributor Author

levitte commented Jan 27, 2024

... with that, I can't see this PR being a draft any more.

@levitte levitte force-pushed the freshen-INSTALL.cmake branch 2 times, most recently from 9dc8357 to 482b851 Compare January 27, 2024 09:46
@bagder bagder requested a review from vszakats January 27, 2024 11:45
@vszakats
Copy link
Member

I'd suggest naming the file INSTALL-CMAKE.md. This matches what we use in that directory and avoids the double-extension.

@levitte
Copy link
Contributor Author

levitte commented Jan 27, 2024

I'd suggest naming the file INSTALL-CMAKE.md. This matches what we use in that directory and avoids the double-extension.

Wish granted!

@levitte
cmake: Freshen up docs/INSTALL.cmake
a7f8357 
- Turn docs/INSTALL.cmake into a proper markdown file, docs/INSTALL.cmake.md
- Move things around to divide the description into configuration, building
  and installing sections
- Mention the more modern cmake options to configure, build and install, but
  also retain the older variants as fallbacks
@vszakats
Copy link
Member

Thank you @levitte, LGTM!

@bagder bagder closed this in 0f4c19b Jan 27, 2024
@bagder
Copy link
Member

bagder commented Jan 27, 2024

Thanks!

@levitte levitte deleted the freshen-INSTALL.cmake branch February 13, 2024 12:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bagder bagder bagder left review comments

@vszakats vszakats vszakats approved these changes

Assignees
No one assigned
Labels
build CI Continuous Integration cmake documentation
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@levitte @vszakats @bagder