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

msi: support path which contains space or parenthesis #589

Merged
merged 1 commit into from
Sep 27, 2023

Conversation

kenhys
Copy link
Contributor

@kenhys kenhys commented Sep 14, 2023

When installing fluent-package to c:\Program Files, the space
character must be escaped correctly.

  • sources.wxs

    • For xml, double quote (") must be "
    • to escape double quote in literal, you must escape with ^&quote;
      it is treated as ^", so it is properly quoted to execute:

    "cmd" /c "fsutil ... "c:\Program Files .." "c:\Program Files...""

  • fluentd.bat,fluent-gem.bat

    • set FOO="..." assign extra double quote, it's troublesome because
      extra double quote will be included when concatenating path.
      The only way to avoid this problem is set "FOO=..." to assign it.
      you can assign to FOO without extra double quote.
    • to get RUBY_VERSION, use "usebackq" and execute command with
      escaped command literal explicitly.
      passed ^""C:\..." ....^" literal are evaluated ^""C:..."^".

@kenhys
Copy link
Contributor Author

kenhys commented Sep 14, 2023

This PR still not work correctly yet. checking log...

@kenhys
Copy link
Contributor Author

kenhys commented Sep 14, 2023

Installed to c:\Program Files:

fluent-escape-space-fluent

Then check fluentdopt:

fluent-escape-space-fluentdopt

Could run manually:

fluent-escape-space-fluentdwinsvc

@kenhys kenhys marked this pull request as ready for review September 14, 2023 09:34
@daipom
Copy link
Contributor

daipom commented Sep 15, 2023

I'm checking the package behavior...

@daipom
Copy link
Contributor

daipom commented Sep 15, 2023

I failed to install the package to C:\Dummy Program Files (FOO)\.
I'm checking the cause.

@daipom
Copy link
Contributor

daipom commented Sep 15, 2023

I checked the log.
InstallFluentdWinSvc is failing.

$  msiexec /i fluent-package-5.0.1-x64.msi /l* installer.log
Action 12:39:40: InstallFluentdWinSvc. 
WixQuietExec64:  \fluent\..\ の使い方が誤っています。
WixQuietExec64:  Error 0x800700ff: Command line returned an error.
WixQuietExec64:  Error 0x800700ff: QuietExec64 Failed
WixQuietExec64:  Error 0x800700ff: Failed in ExecCommon method
CustomAction InstallFluentdWinSvc returned actual error code 1603 (note this may not be 100% accurate if translation happened inside sandbox)
Action ended 12:39:40: InstallFinalize. Return value 3.
Action 12:39:40: Rollback. Rolling back action:
Rollback: InstallFluentdWinSvc
Rollback: Copying new files
Rollback: Creating folders
Rollback: Updating component registration
Action ended 12:40:10: INSTALL. Return value 3.
Action ended 12:40:10: ExecuteAction. Return value 3.
Action 12:40:10: FatalError. 
Action start 12:40:10: FatalError.
Action 12:40:10: FatalError. Dialog created
Action ended 12:40:12: FatalError. Return value 2.
Action ended 12:40:12: INSTALL. Return value 3.

@kenhys
Copy link
Contributor Author

kenhys commented Sep 15, 2023

Are there Executing op: log? /l*v option will enable such feature.

@kenhys
Copy link
Contributor Author

kenhys commented Sep 15, 2023

Are there Executing op: log? /l*v option will enable such feature.

Reproduced. got same log.

@kenhys
Copy link
Contributor Author

kenhys commented Sep 15, 2023

assets/fluentd.bat cause it.

@daipom
Copy link
Contributor

daipom commented Sep 15, 2023

I think this is completely another issue.
I will create another issue later.
We should handle this separately from this PR.

  • This rollback occurs when the path includes ().
  • This is caused by the delayed environment variable mechanism of BATCH.

I think this is the smallest script to reproduce:

/path/to/foo(bar)/test.bat

@echo off
if "a" == "b" (
  echo %~dp0
)
$ .\test.bat
\ was unexpected at this time.

@daipom
Copy link
Contributor

daipom commented Sep 15, 2023

Continue to check the behavior when the installed path including spaces.

@daipom
Copy link
Contributor

daipom commented Sep 15, 2023

bca8ebf

I see!
Certainly, " will solve this problem.

I think this is completely another issue. I will create another issue later. We should handle this separately from this PR.

* This rollback occurs when the path includes `()`.

* This is caused by the `delayed environment variable` mechanism of BATCH.

I think this is the smallest script to reproduce:

/path/to/foo(bar)/test.bat

@echo off
if "a" == "b" (
  echo %~dp0
)
$ .\test.bat
\ was unexpected at this time.

Thanks!

/path/to/foo(bar)/test.bat

@echo off
if "a" == "a" (
  echo "%~dp0"
)
$ .\test.bat
"C:\path\to\foo(bar)\"

@daipom
Copy link
Contributor

daipom commented Sep 15, 2023

Continue to check the behavior when the installed path including spaces.

Installed path: C:\Dummy Program Files\

Something seems wrong with fluent-gem.
Looks like calling fluent-gem breaking the environment.

Fluent Package Command Prompt with administrative privileges:

C:\Windows\system32>fluentd --version
fluent-package 5.0.1 fluentd 1.16.2 (d5685ada81ac89a35a79965f1e94bbe5952a5d3a)

C:\Windows\system32>fluent-gem --version
'C:\Dummy' は、内部コマンドまたは外部コマンド、
操作可能なプログラムまたはバッチ ファイルとして認識されていません。
'C:\Dummy' は、内部コマンドまたは外部コマンド、
操作可能なプログラムまたはバッチ ファイルとして認識されていません。

C:\Windows\system32>fluent-gem --version
C:/Dummy Program Files/fluent/lib/ruby/3.2.0/rubygems.rb:263:in `find_spec_for_exe': can't find gem fluentd (>= 0.a) with executable fluent-gem (Gem::GemNotFoundException)
        from C:/Dummy Program Files/fluent/lib/ruby/3.2.0/rubygems.rb:282:in `activate_bin_path'
        from C:/Dummy Program Files/fluent/bin/fluent-gem:32:in `<main>'

C:\Windows\system32>fluentd --version
C:/Dummy Program Files/fluent/lib/ruby/3.2.0/rubygems.rb:263:in `find_spec_for_exe': can't find gem fluentd (>= 0.a) with executable fluentd (Gem::GemNotFoundException)
        from C:/Dummy Program Files/fluent/lib/ruby/3.2.0/rubygems.rb:282:in `activate_bin_path'
        from C:/Dummy Program Files/fluent/bin/fluentd:32:in `<main>'

@kenhys
Copy link
Contributor Author

kenhys commented Sep 15, 2023

Yes fluent-gem also has same issue.

@daipom
Copy link
Contributor

daipom commented Sep 15, 2023

Oh, is it the same issue?
I don't completely understand the mechanism...
OK, I will check the behavior again after the new package is built!

@kenhys
Copy link
Contributor Author

kenhys commented Sep 15, 2023

f1dd645 has still issues...

@kenhys kenhys force-pushed the msi-escape-space branch 2 times, most recently from 6dea9fc to 425c6b5 Compare September 15, 2023 08:38
@kenhys
Copy link
Contributor Author

kenhys commented Sep 15, 2023

It should be delims=.

@kenhys
Copy link
Contributor Author

kenhys commented Sep 15, 2023

Now fluent-gem will work.

fluent-escape-space-fluent-gem

@daipom
Copy link
Contributor

daipom commented Sep 19, 2023

Thanks. I'm checking the package.

Finally, I understood the mechanism of \ was unexpected at this time. error.

/path/to/foo(bar)/test.bat

@echo off
if "a" == "b" (
  echo %~dp0
)
$ .\test.bat
\ was unexpected at this time.

delayed environment variable has nothing to do with this error.

Is is simply because the () of the path breaks the if () block.

In the above case, echo %~dp0 will be echo /path/to/foo(bar)/.
So, the script will be equivalent to the following.

@echo off
if "a" == "b" (
  echo /path/to/foo(bar)/
)

The error occurs simlply becaues this script is broken.
We can workaround this by ".

@daipom
Copy link
Contributor

daipom commented Sep 25, 2023

Hmm, I thought there was no problem, but I have noticed that I can not run Fluentd in Fluent Package Command Prompt...
I'm checking the cause.

$ fluentd
C:/Dummy Program Files (FOO)/fluent/lib/ruby/gems/3.2.0/gems/fluentd-1.16.2/lib/fluent/config.rb:43:in `initialize': Invalid argument @ rb_sysopen - "C:/Dummy Program Files (FOO)/fluent//etc/fluent/fluentd.conf" (Errno::EINVAL)
        from C:/Dummy Program Files (FOO)/fluent/lib/ruby/gems/3.2.0/gems/fluentd-1.16.2/lib/fluent/config.rb:43:in `open'
        from C:/Dummy Program Files (FOO)/fluent/lib/ruby/gems/3.2.0/gems/fluentd-1.16.2/lib/fluent/config.rb:43:in `build'
        from C:/Dummy Program Files (FOO)/fluent/lib/ruby/gems/3.2.0/gems/fluentd-1.16.2/lib/fluent/supervisor.rb:682:in `setup_global_logger'
        from C:/Dummy Program Files (FOO)/fluent/lib/ruby/gems/3.2.0/gems/fluentd-1.16.2/lib/fluent/supervisor.rb:624:in `configure'
        from C:/Dummy Program Files (FOO)/fluent/lib/ruby/gems/3.2.0/gems/fluentd-1.16.2/lib/fluent/command/fluentd.rb:351:in `<top (required)>'
        from <internal:C:/Dummy Program Files (FOO)/fluent/lib/ruby/3.2.0/rubygems/core_ext/kernel_require.rb>:85:in `require'
        from <internal:C:/Dummy Program Files (FOO)/fluent/lib/ruby/3.2.0/rubygems/core_ext/kernel_require.rb>:85:in `require'
        from C:/Dummy Program Files (FOO)/fluent/lib/ruby/gems/3.2.0/gems/fluentd-1.16.2/bin/fluentd:15:in `<top (required)>'
        from C:/Dummy Program Files (FOO)/fluent/bin/fluentd:32:in `load'
        from C:/Dummy Program Files (FOO)/fluent/bin/fluentd:32:in `<main>'

@kenhys
Copy link
Contributor Author

kenhys commented Sep 26, 2023

Thanks! I'll check it.

@kenhys
Copy link
Contributor Author

kenhys commented Sep 26, 2023

d9b8999 causes msi rollback unexpectedly.

@daipom
Copy link
Contributor

daipom commented Sep 26, 2023

Isn't it caused by not applying this?

#589 (comment)

@kenhys
Copy link
Contributor Author

kenhys commented Sep 26, 2023

Isn't it caused by not applying this?

maybe that's it. Thanks.

@kenhys
Copy link
Contributor Author

kenhys commented Sep 26, 2023

With e26df3f, surely unexpected rollback was fixed.

image

And more, from fluent package prompt with admin also works.

image

@daipom
Copy link
Contributor

daipom commented Sep 26, 2023

What do you think about #589 (comment)?

@kenhys
Copy link
Contributor Author

kenhys commented Sep 26, 2023

It seems work as expected.

@daipom
Copy link
Contributor

daipom commented Sep 26, 2023

Thanks! I will check the built package briefly, just in case.

Copy link
Contributor

@daipom daipom left a comment

Choose a reason for hiding this comment

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

I have confirmed that the build package works as expected! Thanks!

Could you update the title and initial comment?
About the title, since this fix is not only for paths containing space, which contains space or brackets or something like that would be better, I think.

@kenhys
Copy link
Contributor Author

kenhys commented Sep 27, 2023

Thanks, I'll fix it.

@kenhys kenhys changed the title msi: support path which contains space msi: support path which contains space or parenthesis Sep 27, 2023
When installing fluent-package to c:\Program Files, the space
character must be escaped correctly.

* sources.wxs
  * For xml, double quote (") must be &quot;
  * to escape double quote in literal, you must escape with ^&quote;
    it is treated as ^", so it is properly quoted to execute:

  "cmd" /c "fsutil ... "c:\Program Files .." "c:\Program Files...""

* fluentd.bat,fluent-gem.bat
  * set FOO="..." assign extra double quote, it's troublesome because
  extra double quote will be included when concatenating path.
  The only way to avoid this problem is set "FOO=..." to assign it.
  you can assign to FOO without extra double quote.
  * to get RUBY_VERSION, use "usebackq" and execute command with
    escaped command literal explicitly.
     passed `^""C:\..." ....^"` literal are evaluated ^""C:\..."^".

Signed-off-by: Kentaro Hayashi <hayashi@clear-code.com>
@kenhys
Copy link
Contributor Author

kenhys commented Sep 27, 2023

Revised commit message.

@daipom daipom merged commit b4b7017 into fluent:master Sep 27, 2023
19 of 21 checks passed
@daipom
Copy link
Contributor

daipom commented Sep 27, 2023

Thanks!

@daipom daipom added this to the 5.0.2 (T.B.D) milestone Sep 28, 2023
@kenhys kenhys deleted the msi-escape-space branch February 20, 2024 08:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

td-agent not starting as Windows service if installed in directory containing spaces, e.g. "Program Files"
2 participants