edvin / fxlauncher

Auto updating launcher for JavaFX Applications
Apache License 2.0
715 stars 107 forks source link

Adding Support for UNC paths for App URI #145

Closed TurekBot closed 5 years ago

TurekBot commented 5 years ago

Well, I'm quite new at this.

In the following commits, I went and sought out each usage of uri.resolve() and changed it to use instead URI.create(). The reason for this is that uri.resolve() calls uri.normalize() and uri.normalize() breaks UNC paths, as described in greater detail in Issue #143.

It sure took me some doing to test it. I tried to include my edited version as a dependency to a test project, but the gradle plugin, kind of ignored that and used the version it was configured to use. If possible it would be nice to make the gradle plugin more friendly to to using a specifiable version of fxlauncher (maybe it already it is, and I just don't know how).

I ended up cloning the gradle plugin repo, too, then I realized I needed a Groovy SDK, so I installed a Groovy SDK. Then I tried to change the fxlauncherVersion to the custom one I built, but then I couldn't figure out how to get the plugin to install itself into my Local Maven Repository. Running the install task resulted in

FAILURE: Build failed with an exception.

* What went wrong:
Execution failed for task ':signArchives'.
> Cannot perform signing task ':signArchives' because it has no config
ured signatory

* Try:
Run with --stacktrace option to get the stack trace. Run with --info o
r --debug option to get more log output.

BUILD FAILED

So I tried to manually install it, by copying over the jar myself. (Any advice on how I could've gotten the plugin to install itself is welcome. Did I need to disable the signArchives task somehow? AM I DOING ANY OF THIS RIGHT? :sweat_smile: )

Regardless, I finally had a custom fxlauncher and then a custom gradle plugin, and I built my test app and... it worked! It pulled from a UNC path, no problems!

But yeah, I new at this, so do take a good hard look, and please do let me know how I should have done it.

edvin commented 5 years ago

Very nice work! I've added a few comments, could you do a quick commit with those changes before we merge?

You basically did what you had to do to set up the build environment, good job :) I know very little about gradle, but I think we should add an option called skipSigning or something, so that this bit is made conditional:

beforeDeployment { MavenDeployment deployment -> signing.signPom(deployment) }

I'm not sure how to do that though.

TurekBot commented 5 years ago

I can't seem to find your comments. I looked at each of the commits, but couldn't find any comments.

Where might you have left them?

edvin commented 5 years ago

Seems I had to "submit" them before you can see them :) They should show up now, sorry about that!

edvin commented 5 years ago

Great work! I merged, but made a small contribution afterwards:

public class Strings {
    public static String ensureEndingSlash(String s) {
        if (s != null && !s.endsWith("/"))
            s += "/";

        return s;
    }
}

I changed all places where we need to add an ending / so there is less code duplication.

TurekBot commented 5 years ago

Thanks! I like that; much better.