Skip to content

fix: generate timefold-build.properties in the module's build directory (#2629) - #2696

Merged
rsynek merged 2 commits into
TimefoldAI:mainfrom
Sumit-Ks1:fix/generate-build-properties-in-module-directory
Oct 14, 2026
Merged

rsynek merged 2 commits into
TimefoldAI:mainfrom
Sumit-Ks1:fix/generate-build-properties-in-module-directory

Conversation

@Sumit-Ks1

Copy link
Copy Markdown
Contributor

Summary

Fixes #2629 where timefold-build.properties was generated relative to the working directory (Paths.get("target", ...)) rather than the module's build directory.

In a multi-module build or when running Maven from a parent directory, the properties file ended up in the parent's target directory (A/target/...) rather than the submodule's target directory (A/B/target/...).

Furthermore, the Quarkus deployment consumer (TimefoldBuildConfigOverrides) previously suffered from the identical bug, reading relative to the JVM working directory. Fixing only the writer would have silently broken the reader in multi-module builds.

Changes

  1. ConfigureMojo:
    • Resolves the properties file path using the module's buildDirectory (inherited from AbstractPlatformModelMojo, defaulting to ${project.build.directory}) instead of Paths.get("target", ...).
    • Publishes the resolved absolute path as the timefold.build.properties.location system property.
    • Clears the system property at the start of execute() to avoid stale values across reactor modules.
  2. TimefoldBuildConfigOverrides:
    • Reads timefold.build.properties.location first if set.
    • Retains fallback to Paths.get("target", ...) for backwards compatibility when the system property is not present (e.g. standalone test runs or older builds).
  3. Tests:
    • Updated ConfigureMojoTest to test custom build directory resolution, system property publication, and lifecycle cleanup.
    • Added TimefoldBuildConfigOverridesTest to verify handover via system property, fallback behavior, missing/unreadable files, and cross-module constant consistency.
  4. Documentation:
    • Updated README.adoc to reference ${project.build.directory} and document the handover mechanism.

Closes #2629

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot review overview

🟡 Changes recommended

One or more issues must be addressed before approval.

Review effort: Lite
Findings: 1 High severity

Open (1)
What changed in this PR

Fixes multi-module path resolution for generated platform build properties and hands the absolute path to Quarkus.

Changes:

  • Uses ${project.build.directory} for generated properties.
  • Adds system-property handoff and fallback resolution.
  • Expands tests and documentation.
File Description
service/​tools/​maven-plugin/​src/​test/​java/​ai/​timefold/​solver/​tools/​maven/​ConfigureMojoTest.java Updated as part of this pull request.
service/​tools/​maven-plugin/​src/​main/​java/​ai/​timefold/​solver/​tools/​maven/​ConfigureMojo.java Updated as part of this pull request.
service/​tools/​maven-plugin/​src/​main/​java/​ai/​timefold/​solver/​tools/​maven/​AbstractPlatformModelMojo.java Updated as part of this pull request.
service/​tools/​maven-plugin/​README.adoc Updated as part of this pull request.
service/​quarkus/​deployment/​src/​test/​java/​ai/​timefold/​solver/​service/​quarkus/​deployment/​config/​TimefoldBuildConfigOverridesTest.java Updated as part of this pull request.
service/​quarkus/​deployment/​src/​main/​java/​ai/​timefold/​solver/​service/​quarkus/​deployment/​config/​TimefoldBuildConfigOverrides.java Updated as part of this pull request.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

@TomCools

TomCools commented Oct 4, 2026

Copy link
Copy Markdown
Contributor

@Sumit-Ks1 thank you for this contribution. @rsynek I am assigning you to this since you reported the issue.

ConfigureMojo resolved the output path with Paths.get(target, ...), which is relative to the JVM working directory (where mvn was launched), not to the module being built. In a multi-module build or when running from a parent directory, the file ended up in the wrong target directory.

The consumer (TimefoldBuildConfigOverrides) used the same broken relative path. Both sides being wrong in the same way made the bug invisible, but fixing only the writer would silently break the reader.

Changes:

- ConfigureMojo now resolves the path from buildDirectory (inherited from AbstractPlatformModelMojo, defaulting to project.build.directory)

- The absolute path is published as timefold.build.properties.location System property

- TimefoldBuildConfigOverrides reads that property, falling back to the old relative path for backwards compatibility

- The property is cleared on every execute() entry to prevent stale values from a previous reactor module

- Tests cover multi-module path resolution, property lifecycle, and both modules assert the shared literal

- README.adoc documents the corrected path and the handover mechanism

Known limitation (pre-existing, not introduced): parallel reactor builds (mvn -T) can race on the single System property. The generated files are correct per module, but the handover property can be overwritten.

Closes TimefoldAI#2629
@rsynek
rsynek force-pushed the fix/generate-build-properties-in-module-directory branch from d3fb747 to 7479480 Compare October 13, 2026 18:55
Copilot AI lite review requested due to automatic review settings October 13, 2026 18:55

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

One or more issues must be addressed before approval.

1 open finding
1 resolved since last review

🧠 Review effort: Lite

@rsynek rsynek left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks @Sumit-Ks1! One suggestion inline, otherwise looks very good.

Comment thread service/tools/maven-plugin/README.adoc Outdated
Co-authored-by: Radovan Synek <radovan.synek@gmail.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🔵 Needs a closer look

Add coverage proving the no-argument fallback reads the expected generated properties path.

0 open findings

1 resolved since last review

🧠 Review effort: Lite

@rsynek
rsynek merged commit 13aa88b into TimefoldAI:main Oct 14, 2026
21 of 23 checks passed

This branch is waiting to be deployed

1 active (outdated) and 1 waiting deployments
external — 9fcb9648 Waiting Oct 14, 2026 by Sumit-Ks1 via approval_required #1029
internal — 74794807 Deployed Oct 13, 2026 by rsynek via approval_required #1026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Generate timefold-build.properties in the correct maven module

4 participants