Skip to content

Simplifies/Unifies settings + JUnit tests + auto JDK download #258

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

Merged
merged 59 commits into from
Nov 4, 2022
Merged

Conversation

Osiris-Team
Copy link
Collaborator

@Osiris-Team Osiris-Team commented Aug 19, 2022

Contains following breaking changes:

  • Gradle users must encapsulate their task settings inside a javapackager extension, except for the win/linux/mac configs, manfiest and scripts closures.
Show example
task packageMyApp(type: io.github.fvarrui.javapackager.GradlePackageTask, dependsOn: build) {
    javapackager{
        // mandatory
        mainClass = 'path.to.your.mainClass'
        // optional
        bundleJre = true|false
        generateInstaller = true|false
        administratorRequired = true|false
        platform = auto|linux|mac|windows
        additionalResources = [ file('file path'), file('folder path'), ... ]
    }
    linuxConfig {
        ...
    }
    macConfig {
        ...
    }
    winConfig {
        ...
    }
    manifest {
        ....
    }
    scripts {
        ...
    }
    ...
}
  • Maven users would need to rename their name property to appName.

@Osiris-Team
Copy link
Collaborator Author

Osiris-Team commented Aug 23, 2022

image
Tried implementing only the Task, but that also fails with a class cast exception at the line above: at org.gradle.api.internal.project.taskfactory.TaskFactory.create(TaskFactory.java:79)

As you can see, it specifically requires the class to be extended by AbstractTask.

@Osiris-Team
Copy link
Collaborator Author

I got a question about these in my gradle task:
image

Didnt quite understand what closures are and why they are necessary.

Not sure if it works right now, since those methods are inside the gradle package task class and not in the actual extension/settings.

@fvarrui
Copy link
Collaborator

fvarrui commented Aug 24, 2022

Didnt quite understand what closures are and why they are necessary.

Not sure if it works right now, since those methods are inside the gradle package task class and not in the actual extension/settings.

Closures make easier to initialize complex objects, such as WinConfig, MacConfig, LinuxConfig, Scripts, ...:

task packageForMac(type: GradlePackageTask, dependsOn: build) {
        description = 'Packages the application as a native Mac OS app and bundles it in a tarball'
	javapackager{
		platform = 'mac'
		createTarball = true
		description = 'Hello World app'
	}
        scripts {
                bootstrap = file('assets/bootstrap.sh')
        }
        macConfig {
                infoPlist.additionalEntries = '''
                        <key>LSUIElement</key>
                        <true/>
                '''
        }
}

Closures are defined in the Task, not in the JavaPackager extension, so, they should be specified this way.

Task description and plugin description property are not the same thing ... so, name and description properties had to be renamed to appName and appDescription in PackagerSettings.

@Osiris-Team
Copy link
Collaborator Author

Ah ok gotta update the docs.

@fvarrui
Copy link
Collaborator

fvarrui commented Aug 24, 2022

I plan to add new MOJOs and Tasks, in order to split packaging logic in several goals/tasks (create-dmg, create-pkg, create-mac-app, create-linux-app, create-deb, create-rpm, create-win-app, create-setup, create-msi), so it's possible to use this funtionality independiently, allowing developers to define their own packaging workflow. It allows to make changes in the generated app or create your own app structure, before package it in an installation artifact. I mean, devs can create the Mac OS app, make changes on it, and then generate a DMG file. Right now it's not possible.

So, I have a question for you ... how would you implement those new tasks or MOJOs?

@fvarrui
Copy link
Collaborator

fvarrui commented Aug 24, 2022

Maybe, instead of merge all settings in one class,

Ah ok gotta update the docs.

I think it's not clear ... should those closures be inside javapackager extension?

@Osiris-Team
Copy link
Collaborator Author

Well since we now got a lot of stuff simplified you could, easily duplicate the GradlePackageTask for example, rename it to GradleCreateDmgTask, and then register it in the PackagePlugin class.

For compatibility, I would still have a single package task that does all the stuff like before.

@fvarrui
Copy link
Collaborator

fvarrui commented Aug 24, 2022

Well since we now got a lot of stuff simplified you could, easily duplicate the GradlePackageTask for example, rename it to GradleCreateDmgTask, and then register it in the PackagePlugin class.

For compatibility, I would still have a single package task that does all the stuff like before.

But this task doesn't need all those properties ... just a few and/or expose internal artifact generator properties?

@Osiris-Team
Copy link
Collaborator Author

Osiris-Team commented Aug 24, 2022

I think it's not clear ... should those closures be inside javapackager extension?

Well they don't really need to, I think it would make the refactoring for users easier since then there arent any exceptions aka settings that need to be inside of the task scope and not the extension scope.

Idk if that would work anyways since then we might run into maven problems again.

@fvarrui
Copy link
Collaborator

fvarrui commented Aug 24, 2022

Maybe we can use an object mapper like JMapper or Dozer to copy all properties from Task/Mojo to the packager, instead of use the same settings for all tasks

@fvarrui
Copy link
Collaborator

fvarrui commented Aug 24, 2022

Sorry, I know you have been working hard on this stuff, but I need to think more about all this, because I implemented this part of the plugin (to support Gradle/Maven) months or even years ago and I need to refresh my ideas and understand why things were done the way they were done at the time, and see if the new changes are worth it or not. Thanks!!!

@fvarrui
Copy link
Collaborator

fvarrui commented Aug 24, 2022

I know I'm stubborn, but I don't like the idea that refactoring the plugin will change the way you use it

@Osiris-Team
Copy link
Collaborator Author

Maybe we can use an object mapper like JMapper or Dozer to copy all properties from Task/Mojo to the packager, instead of use the same settings for all tasks

Never used an object mapper, but if you say its possible then it should be ^^ The changes I did should at least make it easier to implement a customizable package task.

Sorry, I know you have been working hard on this stuff, but I need to think more about all this, because I implemented this part of the plugin (to support Gradle/Maven) months or even years ago and I need to refresh my ideas and understand why things were done the way they were done at the time, and see if the new changes are worth it or not. Thanks!!!

Yeah, no worries ;)

@Osiris-Team
Copy link
Collaborator Author

Osiris-Team commented Aug 24, 2022

I know I'm stubborn, but I don't like the idea that refactoring the plugin will change the way you use it

There is still the possibility of generating java source code, aka generating all the individual maven/gradle classes from a single class (or json file etc.) with defaults. I tried that, but it was way more work than it was worth.

It would be like a plugin to generate unified maven/gradle plugins.

@fvarrui
Copy link
Collaborator

fvarrui commented Sep 1, 2022

Hi @Osiris-Team!

There is still the possibility of generating java source code, aka generating all the individual maven/gradle classes from a single class (or json file etc.) with defaults. I tried that, but it was way more work than it was worth.

It would be like a plugin to generate unified maven/gradle plugins.

I'm pretty sure your changes have been a great effort to make the plugin easier to maintain, but for now I don't want to cross this line: change the way the plugin is used. As I already told you, I plan to add new tasks/MOJOs, and each one should have its own properties.

So, I prefer the option of generating code from a model, as it will make easier to create new tasks and MOJOs. I think we only have to generate an abstract class for each task and MOJO with the same properties, and then implement these classes, so we don't have to touch them, as they will be regenerated each time the plugin is validated by Gradle, or something so (just before the code is compiled). Maybe we can use javapoet, jcodemodel, or simply Velocity templates.

I'll do some more research about this and share my conclusions here ASAP.

I'm going to merge only the part of the code used to download JDKs and skip the refactored part for now, because there are quite a few pending issues to be resolved and I'm a bit stuck.

Thanks so much!!!

@fvarrui
Copy link
Collaborator

fvarrui commented Nov 5, 2022

Sorry ... I don't know what the hell I did. I merged this PR in the wrong branch (devel) and then reverted changes, so it created a new branch called revert-258-pr-248, but I'm not sure if your changes are there. 🤦

@Osiris-Team
Copy link
Collaborator Author

Osiris-Team commented Nov 5, 2022

Its ok Im gonna delete this and the reverted branch (u gotta do this), since I added the changes to the devel branch of my fork.
I think they are better placed there anyways.
https://github.com/Osiris-Team/JavaPackager/tree/devel

@Osiris-Team Osiris-Team deleted the pr-248 branch November 5, 2022 13:08
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.

2 participants