Skip to content

Comments

864-Windows-installer-issue-fix#1319

Merged
ann0see merged 1 commit intojamulussoftware:masterfrom
henkdegroot:864-Windows-installer-issue
Mar 21, 2021
Merged

864-Windows-installer-issue-fix#1319
ann0see merged 1 commit intojamulussoftware:masterfrom
henkdegroot:864-Windows-installer-issue

Conversation

@henkdegroot
Copy link
Contributor

Resolved the windows installer issue when space exists in the path

@ann0see ann0see self-requested a review March 21, 2021 09:33
@ann0see ann0see self-assigned this Mar 21, 2021
@ann0see ann0see linked an issue Mar 21, 2021 that may be closed by this pull request
@ann0see ann0see added this to the Release 3.7.1 milestone Mar 21, 2021
Copy link
Member

@ann0see ann0see left a comment

Choose a reason for hiding this comment

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

Thank you. I'll test it soon. See my comments which are just small hints.

@henkdegroot henkdegroot force-pushed the 864-Windows-installer-issue branch from eb20774 to a57605f Compare March 21, 2021 11:43
@ann0see
Copy link
Member

ann0see commented Mar 21, 2021

Although it's not needed, but could you also please combine the installer and uninstaller fix into one commit and add "Fixes #864" into the commit description?

@ann0see ann0see self-requested a review March 21, 2021 12:51
Copy link
Member

@ann0see ann0see left a comment

Choose a reason for hiding this comment

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

Built it in a path with one space (and also in one with multiple ones), and I can confirm it works now. Thank you very much for solving this issue. Hopefully there aren't any side effects from adding delims=?

Note to maintainers:

txvContributors->setText (
since you're a new contributor, he needs to be added.

@ann0see ann0see requested review from gilgongo and pljones March 21, 2021 13:14
@ann0see ann0see added the bug Something isn't working label Mar 21, 2021
@henkdegroot
Copy link
Contributor Author

@ann0see I checked the delims option and by not specifying a delimiter it ignores any. My first test was setting a specific delimiter but that would have introduced the same issue when that character is uses.

@henkdegroot henkdegroot force-pushed the 864-Windows-installer-issue branch from a57605f to 1d8379f Compare March 21, 2021 15:46
@henkdegroot
Copy link
Contributor Author

Although it's not needed, but could you also please combine the installer and uninstaller fix into one commit and add "Fixes #864" into the commit description?

I have combined into one commit now. Hoping I did it correct, first time I have done this.

@ann0see
Copy link
Member

ann0see commented Mar 21, 2021

Yep. That's ok.

You can add a description to every commit (not only a message description), but that's not needed.

Copy link
Collaborator

@pljones pljones left a comment

Choose a reason for hiding this comment

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

Approving untested -- the quoting does look very peculiar and c-n-p to a .bat file got a complaint before it got as far as expanding the variables. I'll take @ann0see's and @henkdegroot's word that it works in the NSI :)

@ann0see ann0see merged commit fdd0f02 into jamulussoftware:master Mar 21, 2021
@ann0see
Copy link
Member

ann0see commented Mar 21, 2021

Just merged it. Thank you again! You can add yourself as contributor here:

txvContributors->setText (

And could also open an issue on the documentation repo and state that the mentioning of the spaces issue can be removed from the compiling file.

@henkdegroot henkdegroot deleted the 864-Windows-installer-issue branch March 22, 2021 06:46
@henkdegroot
Copy link
Contributor Author

Just create my 2nd PR....to add myself to the contributors :)

Will now create the issue on the documentation repo to get this updated.

@ann0see
Copy link
Member

ann0see commented Mar 22, 2021

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Windows installer doesn't build correctly in a path with spaces

3 participants