Skip to content

Conversation

@codeconsole
Copy link
Contributor

@codeconsole codeconsole commented Oct 27, 2025

grails. is stripped out of all build settings when the jvm is forked.

This correctly sets the TARGET_DIR after it has been stripped by using project.target.dir

Difference from 7.0.0

TARGET_DIR = new File(BASE_DIR, 'build')

to

TARGET_DIR = new File(BASE_DIR, System.getProperty(PROJECT_TARGET_DIR.substring(7)) ?: 'build')

@codeconsole
Copy link
Contributor Author

#15127

@matrei
Copy link
Contributor

matrei commented Oct 28, 2025

Why is grails. stripped in the first place in configureForSettings?

The message of the commit that added that code in 2015 is:
Infrastructure support to allow changing server port and enable HTTPS from command line

That implies to me that this is done to convert grails.server.port -> server.port and grails.server.port.https -> server.port.https and possibly some other properties.

Would it not be more transparent to explicitly convert the Spring Boot properties that we care about in configureForkSettings instead of bulk converting every system property that starts with grails.?

@codeconsole
Copy link
Contributor Author

codeconsole commented Oct 28, 2025

Why is grails. stripped in the first place in configureForSettings?

The message of the commit that added that code in 2015 is: Infrastructure support to allow changing server port and enable HTTPS from command line

That implies to me that this is done to convert grails.server.port -> server.port and grails.server.port.https -> server.port.https and possibly some other properties.

@matrei this PR isn't stripping anything. All this PR does is reference the historically correct target.dir. The plugin has always stripped them. I started down the path of not stripping them, but since it has always been like that, it might be beyond the scope for 7.x

Would it not be more transparent to explicitly convert the Spring Boot properties that we care about in configureForkSettings instead of bulk converting every system property that starts with grails.?

yes, I agree with everything you are saying, but we should prob not be that invasive until 8.x, no?

@codeconsole
Copy link
Contributor Author

codeconsole commented Oct 28, 2025

An alternative for this PR is not reference the constant at all.

TARGET_DIR = new File(BASE_DIR, System.getProperty('target.dir') ?: 'build')

BASE_DIR = System.getProperty(APP_BASE_DIR) ? new File(System.getProperty(APP_BASE_DIR)) : (IOUtils.findApplicationDirectoryFile() ?: new File('.'))
GRAILS_APP_DIR_PRESENT = new File(BASE_DIR, 'grails-app').exists() || new File(BASE_DIR, 'Application.groovy').exists()
TARGET_DIR = System.getProperty(PROJECT_TARGET_DIR) ? new File(BASE_DIR, System.getProperty(PROJECT_TARGET_DIR)) : new File(BASE_DIR, 'build')
TARGET_DIR = new File(BASE_DIR, System.getProperty(PROJECT_TARGET_DIR.substring(7)) ?: 'build')
Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with your comment, in this case it's better to not use the constant.
Also System.getProperty has a version that takes a default value:

System.getProperty('project.target.dir', 'build')

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, that's what I meant.

@jamesfredley jamesfredley added this to the grails:7.0.1 milestone Oct 29, 2025
@jamesfredley jamesfredley moved this to In Progress in Apache Grails Oct 29, 2025
@jdaugherty jdaugherty merged commit 3bd608a into apache:7.0.x Oct 29, 2025
36 checks passed
@github-project-automation github-project-automation bot moved this from In Progress to Done in Apache Grails Oct 29, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

4 participants