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

Specify full entrypoint name on crash #928

Merged
merged 3 commits into from
Aug 15, 2024
Merged

Conversation

SHsuperCM
Copy link
Contributor

Due to the way I use entrypoints in my projects I tend to encounter entrypoint crashes often while developing.
The current logging makes it hard to distinguish which entrypoint is being problematic at a glance.

This small change in fabric loader specifies in the log the exact entrypoint that caused the game to crash.

Essentially changes:

 java.lang.RuntimeException: Could not execute entrypoint stage 'main' due to errors, provided by 'minecraft-test'!
	at net.fabricmc.loader.impl.FabricLoaderImpl.lambda$invokeEntrypoints$2(FabricLoaderImpl.java:388) ~[main/:?]

into:

 java.lang.RuntimeException: Could not execute entrypoint stage 'main' due to errors, provided by 'minecraft-test->(0.3.x)net.fabricmc.minecraft.test.TestEntrypoint::makeCrashPlz'!
	at net.fabricmc.loader.impl.FabricLoaderImpl.lambda$invokeEntrypoints$2(FabricLoaderImpl.java:388) ~[main/:?]

@SHsuperCM
Copy link
Contributor Author

I also dont really know the importance of the (0.3.x) in the new entry's toString so I didnt remove it in this commit until I know more about it and if it is safe to remove.

@SHsuperCM SHsuperCM requested a review from Juuxel May 18, 2024 05:06
@modmuss50 modmuss50 added this to the 0.16.0 milestone Jun 8, 2024
@SHsuperCM
Copy link
Contributor Author

Before this gets merged, I want to reiterate that I would like to get rid of the (0.3.x) as it doesnt add much and could lead to confusion with mod versions. But I am unsure of how it would be best to do so.

Ideally it could just be removed from

return mod.getMetadata().getId() + "->(0.3.x)" + value;

as I'm pretty sure the distinction is no longer relevant in this version but you guys probably know more if that marking is still needed.

@modmuss50
Copy link
Member

Before this gets merged, I want to reiterate that I would like to get rid of the (0.3.x) as it doesnt add much and could lead to confusion with mod versions. But I am unsure of how it would be best to do so.

Ideally it could just be removed from

return mod.getMetadata().getId() + "->(0.3.x)" + value;

as I'm pretty sure the distinction is no longer relevant in this version but you guys probably know more if that marking is still needed.

I see yes, I dont think thats needed at all. seems fine to remove to me. I need to take a proper look at this exisitng code in an IDE before approving the change. It seems fine on the surface though.

@sfPlayer1
Copy link
Contributor

sfPlayer1 commented Jun 9, 2024

I think it'd be nicer to keep the current phrasing and only add the location at the end ([...]by <modid> at <entry.value>), minimizing the reliance on toString's implementation.

@SHsuperCM
Copy link
Contributor Author

I think it'd be nicer to keep the current phrasing and only add the location at the end ([...]by <modid> at <entry.value>), minimizing the reliance on toString's implementation.

By all means, I made the PR to be as minimally invasive on the API as possible so I used what was already there. I used the toString because it seems like referencing the entry by a string is implementation specific and not something required by the entrypoint container itself. (also why I opted for entrypoint "name" rather than "definition")

@SHsuperCM
Copy link
Contributor Author

SHsuperCM commented Jul 14, 2024

Added the requested phrasing. I'm pretty sure this does not break api as entrypoint storages are impl.

@modmuss50 modmuss50 merged commit 12dd25f into FabricMC:master Aug 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants