Skip to content
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

Adding Support for UNC paths for App URI #145

Merged
merged 9 commits into from
Dec 21, 2018
Merged

Conversation

TurekBot
Copy link
Collaborator

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? 😅 )

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
Copy link
Owner

edvin commented Dec 20, 2018

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.

README.md Show resolved Hide resolved
@TurekBot
Copy link
Collaborator Author

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
Copy link
Owner

edvin commented Dec 20, 2018

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

@edvin edvin merged commit 7d6e607 into edvin:master Dec 21, 2018
@edvin
Copy link
Owner

edvin commented Dec 21, 2018

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
Copy link
Collaborator Author

Thanks! I like that; much better.

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