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

minetest rev2 & fix irrlicht 1.9.0mt8 #17241

Closed
wants to merge 1 commit into from

Conversation

Zweihorn
Copy link
Contributor

@Zweihorn Zweihorn commented Jan 7, 2023

This PR was obsoleted by

Description

Combined amend minetest rev2 & fix irrlicht 1.9.0mt8 rev1

ref minetest/irrlicht#152

Type(s)
  • bugfix
  • enhancement
  • security fix
Tested on

macOS 11.7.2 20G1020 x86_64
Command Line Tools 13.2.0.0.1.1638488800

Verification

Have you

  • followed our Commit Message Guidelines?
  • squashed and minimized your commits?
  • checked that there aren't other open pull requests for the same change?
  • referenced existing tickets on Trac with full URL?
  • checked your Portfile with port lint --nitpick?
  • tried a full install with sudo port -vst install?
  • tested basic functionality of all binary files?
  • checked that the Portfile's most important variants haven't been broken?
Future tasks:

The b.m. Trac ticket should not hinder any review and is an independent activity by same maintainer:

@macportsbot
Copy link

Notifying maintainers:
@l2dy for port irrlichtmt.

@Zweihorn
Copy link
Contributor Author

Zweihorn commented Jan 7, 2023

ERROR - This is somewhat unexpected.

. . .
 -- Detecting CXX compile features - done
 -- *** Will build version 5.6.1-MacPorts-rev2 ***
 -- Found IrrlichtMt 1.9.0.9
 CMake Error at CMakeLists.txt:135 (message):
   IrrlichtMt 1.9.0mt8 is required to build
 
 
 -- Configuring incomplete, errors occurred!

AFAIK the a.m. test build should have combined IrrlichtMt 1.9.0.9 with MT 5.6.1-MacPorts-rev2
see #17153 (comment)

However, I have to check again as the port install of MT could have just grabbed the 1.9.0mt8 and thus ignored the new IrrlichtMt 1.9.0.9 build apparently. Anyway not a big problem and I will introduce the a.m. fixes to 1.9.0mt8 as backport.

Yep my fault and should have checked with minetest --version already before:

% /opt/macports-test/Applications/minetest.app/Contents/MacOS/minetest --version
Minetest 5.6.1-MacPorts-rev2 (OSX)
Using Irrlicht 1.9.0mt8
Using LuaJIT 2.1.0-beta3
BUILD_TYPE=Release
RUN_IN_PLACE=0
USE_CURL=1
USE_GETTEXT=1
USE_SOUND=1
STATIC_SHAREDIR="minetest.app/Contents/Resources"
STATIC_LOCALEDIR="minetest.app/Contents/Resources/locale"

A good learning experience again.

@l2dy - Sorry for the newbie noise produced by me.

Back to the drawing board ...

@Zweihorn Zweihorn changed the title update minetest rev2 & irrlicht 1.9.0mt9 minetest rev2 & fix irrlicht 1.9.0mt8 Jan 7, 2023
* backport fixes to irrlicht 1.9.0mt8

* amend MT 5.6.1 rev2
  * introduce new variants: [+]GLES, benchmark, debug, gprof, noclient, psql, server
  * fix minetest/irrlicht#152

* introduce irrlichtmt rev1
  * Fix01: irrlichtmt fails to install on Mac OS 10.6 <https://trac.macports.org/ticket/66439>
  * Fix02: minetest arm64 support <https://trac.macports.org/ticket/66600>
@Zweihorn
Copy link
Contributor Author

Zweihorn commented Jan 7, 2023

Ready for review...

And the skeletons in the drawer are:

@Zweihorn Zweihorn marked this pull request as ready for review January 7, 2023 21:38
@Zweihorn
Copy link
Contributor Author

Zweihorn commented Jan 8, 2023

@aeiouaeiouaeiouaeiouaeiouaeiou

Pleeeeeeeeeaaaaaaase kindly consider approval of review on basis of my a.m. resolve.

Anyway there will be another day with "minetest" rev3 in the future, I presume.

@l2dy
Copy link
Member

l2dy commented Jan 8, 2023

You should split this commit into 2, following the Commit Message Guidelines. Here are some git commands for your reference, with xxxx replaced by the actual commit message.

# in repository root with a clean working tree
git reset --mixed HEAD~1
git commit -m 'irrlichtmt: xxxx' -- devel/irrlichtmt
git commit -m 'minetest: xxxx' -- games/minetest
git push -f

Edit: 1 is the number of commits to move backwards in history, in your PR there is 1.

@Zweihorn
Copy link
Contributor Author

Zweihorn commented Jan 8, 2023

@l2dy - Sorry, but let me ask what was your message in "combining and commit on top" before?

You should split this commit into 2, following the Commit Message Guidelines. Here are some git commands for your reference, with xxxx replaced by the actual commit message.

# in repository root with a clean working tree
git reset --mixed HEAD~1
git commit -m 'irrlichtmt: xxxx' -- devel/irrlichtmt
git commit -m 'minetest: xxxx' -- games/minetest
git push -f

Edit: 1 is the number of commits to move backwards in history, in your PR there is 1.

@l2dy - THX - 😮‍💨 May the Port be with you. 🤣

Apparently, I totally misunderstood and naturally I am to blame and my job of learning and improving as new maintainer. Don't want to be a bother to you, however, I sincerely would have preferred some more clarity in your before message.

Anyway, I already made it much more challenging to you and reviewers due to:

  1. the valid expert's approach to avoid the variant mess;
  2. the evident binding of MT 5.6 to IrrLichtMT8 and MT 5.7 to IrrlichtMT9

Resolving to a new approach with several new ports & Portfiles for both 'ircchlichtmt' and 'minetest' respectively.

There will be fun.

🌻

@Zweihorn Zweihorn closed this Jan 8, 2023
@Zweihorn Zweihorn deleted the minetest_rev2_003 branch January 8, 2023 15:08
@l2dy
Copy link
Member

l2dy commented Jan 8, 2023

@l2dy - Sorry, but let me ask what was your message in "combining and commit on top" before?

A PR could be made from a branch that has more than one commits to be merged. In most cases, it's recommended to isolate changes of different ports into different commits and stack them on top of each other in their order of dependency relationship.

If for any reason this is not feasible, you should still follow the "List any modified ports in Subject" rule and start your commit message with irrlichtmt, minetest: .

@Zweihorn
Copy link
Contributor Author

Zweihorn commented Jan 8, 2023

Old News

If for any reason this is not feasible, you should still follow the "List any modified ports in Subject" rule and start your commit message with irrlichtmt, minetest: .

Good advice for some future use, I guess.

This PR was closed and the branch was deleted one hour ago.

New Deal

On to a new approach with several new ports & Portfiles for both 'irrlichtmt' and 'minetest' respectively and independent commits. All this although a quite interdependent change to both ports will hopefully happen soon.

There will be fun.

🌻

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
4 participants