Skip to content

Control file placeholders not expanded correctly #822

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

Closed
mattdibi opened this issue May 9, 2025 · 15 comments · Fixed by #823
Closed

Control file placeholders not expanded correctly #822

mattdibi opened this issue May 9, 2025 · 15 comments · Fixed by #823

Comments

@mattdibi
Copy link
Contributor

mattdibi commented May 9, 2025

Hello there!

I'm using jdeb version 1.12 to generate debian packages for our project. I have encountered an issue similar to this SO question here.

I need to generate the Debian version depending on the selected Maven profile. Therefore I have:

<profiles>
        <!-- Internal profile: FOR INTERNAL USE ONLY - active if -DreleaseBuild is *not* specified. -->
        <profile>
            <id>debugBuild</id>
            <activation>
                <property>
                    <name>!releaseBuild</name>
                </property>
            </activation>
            <properties>
                <maven.build.timestamp.format>yyyyMMddHHmm</maven.build.timestamp.format>
                <package.snapshot>git${maven.build.timestamp}.${git.commit.id.abbrev}</package.snapshot>

                <package.version>${release.version}~${package.snapshot}</package.version>
                <package.revision>1</package.revision>
            </properties>
        </profile>
        <!-- Internal profile: FOR INTERNAL USE ONLY - active if -DreleaseBuild *is* specified. -->
        <profile>
            <id>releaseBuild</id>
            <activation>
                <property>
                    <name>releaseBuild</name>
                </property>
            </activation>
            <properties>
                <package.version>${release.version}</package.version>
                <package.revision>1</package.revision>
            </properties>
        </profile>
    </profiles>

Where ${git.commit.id.abbrev} and ${release.version} are set by two other Maven plugins in the initialize phase. Essentially we have a Maven CLI parameter that allows us to select the build type:

  • Debug builds: version X.X.X~gitYYYYMMDDHHMM.ZZZZZZZ-1
  • Release builds: version X.X.X-1.

In the control file we have:

Package: [[output.installer.name]]
Version: [[package.version]]-[[package.revision]]
Section: admin
Priority: optional
...

The resulting control file is not correct

apt show ./project.deb
Package: project
Version: ${release.version}~git202505090852.${git.commit.id.abbrev}-1
Section: admin
Priority: optional
...

Is this expected?

If I expand the placeholders using (for example) maven-resources-plugin everything works, therefore the variables are populated correctly at the time jdeb generates the control file. Same goes when using jdeb's snapshotExpand/snapshotTemplate: the variable are expanded properly (unfortunately the snapshotTemplate option is not viable for our development model).

I'm quite confused by this behaviour.

@tcurdt
Copy link
Owner

tcurdt commented May 9, 2025

Thanks for reporting.
jdeb-1.2 is very very old. Have you verified the behaviour with jdeb-1.13?

IIUC the maven variable expansion is not applied as you would expect.

@mattdibi
Copy link
Contributor Author

Hi @tcurdt,

thank you for the quick feedback.

Sorry 1.2 is a typo, I meant to write 1.12. I tried 1.13 anyway but the result is the same:

apt show ./project.deb
Package: project
Version: ${release.version}~git202505100753.${git.commit.id.abbrev}-1
Priority: optional

@mattdibi
Copy link
Contributor Author

Hi @tcurdt,

to help in debugging this issue I created a minimal reproducible example here: https://github.com/mattdibi/jdeb-minimal-example

@tcurdt
Copy link
Owner

tcurdt commented May 10, 2025

Thanks for that!

I think the best next step would be to create an integration test. Similar to this one https://github.com/tcurdt/jdeb/tree/master/src/it/project-build-outputTimestamp

@tcurdt
Copy link
Owner

tcurdt commented May 10, 2025

It's a little odd. But it's been a while since I last look into that.
I would expect maven to expand the variables before providing it to plugins. But looking at this here - it might not.

https://github.com/tcurdt/jdeb/blob/master/src/main/java/org/vafer/jdeb/maven/DebMojo.java#L85

I suspect this must be a tiny fix - but it probably is more of a maven question to fix it.

@mattdibi
Copy link
Contributor Author

mattdibi commented May 10, 2025

Thanks for that!

I think the best next step would be to create an integration test. Similar to this one https://github.com/tcurdt/jdeb/tree/master/src/it/project-build-outputTimestamp

Here you go: #823

This is in no way perfect but reproduces the issue I found. I wonder if it's possible to trigger it without the need for an external plugin 🤔

P.s. I'm not a groovy expert therefore the verify script might require some more work.

It's a little odd. But it's been a while since I last look into that. I would expect maven to expand the variables before providing it to plugins. But looking at this here - it might not.

https://github.com/tcurdt/jdeb/blob/master/src/main/java/org/vafer/jdeb/maven/DebMojo.java#L85

I suspect this must be a tiny fix - but it probably is more of a maven question to fix it.

Not quite clear to me: are you saying that this is caused by maven?

@tcurdt
Copy link
Owner

tcurdt commented May 10, 2025

Thanks for the test! That should make it much easier to figure out.

Not quite clear to me: are you saying that this is caused by maven?

In a way.

These are maven properties. It seems a little strange when a plugin would have to manually resolve them.
I would rather think that maven would resolve them and THEN pass them to the plugin. But that's not what we are seeing here.

So I think there must be a way for the plugin to

  • resolve them via some maven helper
  • ask to receive resolved values from maven

but it's been a long time since I worked with maven on that level.
So it's more some educated guessing at this stage.

We either need to find someone that is more into the maven plugin API or we look at how other plugins (ideally official maven plugins) are doing it. Asking on the dev list (or chat) might also be a good way to figure this out.

mattdibi added a commit to mattdibi/jdeb that referenced this issue May 11, 2025
@mattdibi
Copy link
Contributor Author

Writing here some more observations/experiments (I'm not a maven expert therefore I might write stuff that is obvious to you but bear with me :D):

I logged the content of the maven properties at VariableResolver initialization and, for the integration test introduced in #823, I get the following:

Source changes
diff --git a/src/main/java/org/vafer/jdeb/maven/DebMojo.java b/src/main/java/org/vafer/jdeb/maven/DebMojo.java
index 0bd1daf..cb52e23 100644
--- a/src/main/java/org/vafer/jdeb/maven/DebMojo.java
+++ b/src/main/java/org/vafer/jdeb/maven/DebMojo.java
@@ -526,6 +526,8 @@ public class DebMojo extends AbstractMojo {
 
         final VariableResolver resolver = initializeVariableResolver(new HashMap<String, String>(), outputTimestampMs);
 
+        getLog().info(((Map) getProject().getProperties()).toString());
+
         final File debFile = new File(Utils.replaceVariables(resolver, deb, openReplaceToken, closeReplaceToken));
         final File controlDirFile = new File(Utils.replaceVariables(resolver, controlDir, openReplaceToken, closeReplaceToken));
         final File installDirFile = new File(Utils.replaceVariables(resolver, installDir, openReplaceToken, closeReplaceToken));
[INFO] [INFO] {maven.compiler.target=1.8, release.version=2.0.0, property.project.version=${release.version}-1, property.package.revision=1, maven.compiler.source=1.8, property.upstream.version=2.0.0-SNAPSHOT}
[INFO] [INFO] Creating debian package: /Users/mattia/Desktop/jdeb/target/it/property-expansion/target/jdeb-it_1.0_all.deb
[INFO] [INFO] Building data

We can see that release.version was correctly expanded while property.project.version which references release.version was not.

Given that the content of release.version is correct I thought that the only thing that is missing is an additional round of interpolation during VariableResolver initialization... which indeed works. See: 2af1501

🤔 What do you think @tcurdt is this approach correct?

@tcurdt
Copy link
Owner

tcurdt commented May 11, 2025

That really works? I am a little confused without debugging this. I don't see how.
Because the jdeb VaraiableResolver resolves the jdeb style variables [[var]].
Here is the test for it:

public void testReplaceVariables() {

This topic reminds me a little of the PR #148

We can see that release.version was correctly expanded while property.project.version which references release.version was not.

The question is whether there is a way for maven to do the expansion or not.

If there really is no maven way of doing that (and it would be good to confirm that), it should probably be a new MavenVariableResolver. That would need to be called repeatedly until there are no further variables to resolve (or maximum loop count is reached).

@tcurdt
Copy link
Owner

tcurdt commented May 11, 2025

Ah - now I see you have already added the Plexus code for it in the PR. That's what made it work.

@tcurdt
Copy link
Owner

tcurdt commented May 11, 2025

This looks like a more correct implementation than what is in the PR.
It would just be nice to have some test coverage for this.

@mattdibi
Copy link
Contributor Author

This topic reminds me a little of the PR #148

Actually yeah, the use-case is almost identical. I didn't notice this PR before.

It would just be nice to have some test coverage for this.

I tried a little but I'm having trouble finding the best approach here. Do you have any suggestion?

@tcurdt
Copy link
Owner

tcurdt commented May 11, 2025

I tried a little but I'm having trouble finding the best approach here. Do you have any suggestion?

I would suggest moving the VariableResolver implementation into it's own class (if easily possible that is).
The connection to maven/plexus is what will make the whole testing a bit more complicated.

Before we didn't really have to deal with that. So I don't think there is a good unit test to look at in the jdeb code base.
TBH it might be good to look at the plexus utils. I am sure they have some unit testing. Maybe we can borrow some of that.

While not ideal I could also live with some integration tests though.

The variable expansion has slowly grown to one of most messiest area.
Hence I feel the need to really have some testing in place here before we change things.

@mattdibi
Copy link
Contributor Author

Thanks for the pointers! that's what I needed. I'll take a look at this in the next days (most probably next weekend) :)

@mattdibi
Copy link
Contributor Author

Hello there @tcurdt,

as promised I refactored the code and added unit tests.

The main change is the fact that I moved the initializeVariableResolver inside the build method of the newly introduced MapVariableResolverBuilder, a class implementing the Builder pattern that allows to leverage dependency injection to decouple from maven.

This allows for easy testing of the variable expansion part of the old initializeVariableResolver method.

Feedback welcome! :)

tcurdt pushed a commit that referenced this issue May 18, 2025
* test: add baseline

* test: trigger issue

* test: verify control file content

* test: remove debug prints

* fix: issue #822

* refactor: move initializeVariableResolver into MapVariableResolver builder

* refactor: remove unnecessary arguments

* refactor: remove unused imports

* test: initial sanity check unit test

* test: more comprehensive testing

* test: refactor for easier readability

* test: added nested meta property

* test: add system properties interpolation test

* test: minor fixes

* style: retab

* style: consistent use of "this"

* style: fix copyright header year

* test: add test case for optional properties

* test: add test case with minimal data

* test: improved naming convention

* feat: add some error handling

* refactor: remove getMap()
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants