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

Build message information on how to build with in-tree Irrlicht #11581

Merged
merged 2 commits into from
Sep 5, 2021
Merged

Build message information on how to build with in-tree Irrlicht #11581

merged 2 commits into from
Sep 5, 2021

Conversation

20kdc
Copy link
Contributor

@20kdc 20kdc commented Aug 31, 2021 •
edited
Loading

  • Helps improve first-build experience without altering project structure.
  • Changes the reported message when IrrlichtMt is not found.
  • This PR specifically aids in helping people compile Minetest locally without performing a system-wide installation of IrrlichtMt.

To do

This PR is Ready for Review.

How to test

  • Confirm that the resulting change is parsable by CMake. (EDIT: To be clear, it works fine for me, but I'm specifically on cmake version 3.16.3)
  • Confirm that the wording is sufficiently clear and that formatting is ideal.
  • Confirm that the instructions are the standard that ought to be used for new contributors and/or people who need to make local builds due to system circumstances.

@20kdc
Build message information on how to build with in-tree Irrlicht
1b45e20 
Commit written via GitHub web UI for simplicity's sake
JosiahWI
JosiahWI reviewed Aug 31, 2021
View reviewed changes
CMakeLists.txt Outdated
"It can be found here: https://github.com/minetest/irrlicht")
"The Minetest team has forked Irrlicht to make their own customizations.\n"
"It can be found here: https://github.com/minetest/irrlicht\n"
"For example: git clone --depth=1 https://github.com/minetest/irrlicht lib/irrlichtmt\n")
Copy link
Contributor

Choose a reason for hiding this comment

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

Does --depth=1 matter here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not particularly, but it speeds up the clone.

JosiahWI
JosiahWI approved these changes Sep 1, 2021
View reviewed changes
Copy link
Contributor

@JosiahWI JosiahWI left a comment

Choose a reason for hiding this comment

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

Looks good to me.

SmallJoker
SmallJoker approved these changes Sep 1, 2021
View reviewed changes
Copy link
Member

@SmallJoker SmallJoker left a comment

Choose a reason for hiding this comment

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

Simple & helpful. I think this is fine.

@SmallJoker SmallJoker added One approval ✅ ◻️ Trivial The change is a trivial bug fix, documentation or maintenance change, as per the Git Guidelines labels Sep 1, 2021
sfan5
sfan5 reviewed Sep 1, 2021
View reviewed changes
CMakeLists.txt Outdated Show resolved Hide resolved
@sfan5 sfan5 merged commit a3e32d8 into minetest:master Sep 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sfan5 sfan5 sfan5 approved these changes

@SmallJoker SmallJoker SmallJoker approved these changes

@JosiahWI JosiahWI JosiahWI approved these changes

Assignees
No one assigned
Labels
Trivial The change is a trivial bug fix, documentation or maintenance change, as per the Git Guidelines >= Two approvals ✅ ✅
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@20kdc @sfan5 @SmallJoker @JosiahWI