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

In dev environment, treat groovy/kotlin/gradle template placeholders as wildcards #359

Closed
wants to merge 1 commit into from

Conversation

dexman545
Copy link

So things like "version":"${version}" don't error.

This is particularly useful when variables are used for dependencies, as IDE run configs cannot run processResources to fill in the blanks.

@i509VCB
Copy link
Contributor

i509VCB commented Jan 12, 2021

I would like to ask, should we lock this behind a system property in DLI that loom automatically enables? Since this does step into platform specific territory. Ask Player and modmuss about that before you do this of course.

Also the templates in file also work with kotlin dsl - so this is a general gradle thing.

However doing the system property would require loom updates - so it probably wont work.

@i509VCB i509VCB requested review from a team January 12, 2021 00:57
@dexman545 dexman545 changed the title In dev environment, treat groovy template placeholders as wildcards In dev environment, treat groovy/kotlin/gradle template placeholders as wildcards Jan 12, 2021
@dexman545
Copy link
Author

I'll wait for player/modmuss before doing a system property, but I don't think it's particularly needed as the place holders exist in the general mod.json only in dev environments, and would be useful for more than just MC.

@liach
Copy link
Contributor

liach commented Jan 12, 2021

Imo the right way is to make ide process resources so the placeholders are substituted with actual versions. similarly for gradle, we should make runXxx happen only after processResources which substitutes such vars

@dexman545
Copy link
Author

Imo the right way is to make ide process resources so the placeholders are substituted with actual versions. similarly for gradle, we should make runXxx happen only after processResources which substitutes such vars

This only works for gradle tasks, not the generated run configs.

@liach
Copy link
Contributor

liach commented Jan 12, 2021

Read the first sentence:

Imo the right way is to make ide process resources so the placeholders are substituted with actual versions

There is definitely a way for ide to process resources before running their run tasks. We just need to figure out how to do that; in the end if there is no elegant way in ide we can just make ide run processResources gradle task and make ide use gradle processed resources.

@i509VCB
Copy link
Contributor

i509VCB commented Jan 12, 2021

If you run with intellij the placeholders do not get set. Probably the same if you do what Player does with eclipse for starting the game.

@liach
Copy link
Contributor

liach commented Jan 12, 2021

If you run with intellij the placeholders do not get set.

So yes, we can add resource processing to idea's pre-run tasks. This is a thing.
image

@@ -44,6 +45,11 @@
}
}

// In dev environment, ignore groovy template placeholders - any version is satisfactory
if (FabricLoader.getInstance().isDevelopmentEnvironment() && s.matches("(\\$\\{\\w+})")) {
Copy link
Member

Choose a reason for hiding this comment

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

The regex doesn't match things like $version, which also work with Gradle.

@modmuss50
Copy link
Member

Im not a fan of this at all.

One suggestion/idea we have had for a while is to read the version from the json into gradle.

@dexman545
Copy link
Author

Bad hardcoded task for Idea to be run after build but before launch:

task fixResourcesLaunch(type: Copy) {
	String name = project.name.replace(" ", "_") + ".main"

	destinationDir file("${projectDir}/out/production/${name}")
	from processResources
}

Closing since this doesn't seem wanted.

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.

5 participants