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

fix: electron #685

Merged
merged 3 commits into from
Aug 5, 2020
Merged

fix: electron #685

merged 3 commits into from
Aug 5, 2020

Conversation

koonpeng
Copy link
Collaborator

Workaround ENAMETOOLONG error when running in electron renderer process.

With this PR in electron electron/electron#24659, rclnodejs is able to run in electron's main process and renderer process with allowRendererProcessReuse set to false.

@koonpeng koonpeng marked this pull request as draft July 27, 2020 02:58
koonpeng added 3 commits August 5, 2020 14:26
rcl logging uses `program_invocation_name` to determine the log file,
chromium mangles the program name to include all args, this causes a
ENAMETOOLONG error when starting ros. Workaround is to replace the first
occurence of ' -' with the null terminator. see:
https://unix.stackexchange.com/questions/432419/unexpected-non-null-encoding-of-proc-pid-cmdline
@coveralls
Copy link

Coverage Status

Coverage remained the same at 84.384% when pulling f77f294 on fix/electron into 9523d21 on develop.

@koonpeng
Copy link
Collaborator Author

koonpeng commented Aug 5, 2020

some info about the problem.

electron/chromium changes argv[0] to provide more helpful description in ps, however ros also uses argv[0] to determine the filename for logs. This often causes ros to use a filename that is too long for the system, resulting in ENAMETOOLONG error.

This is a workaround for it that terminates argv[0] on the first " -" occurrence so ros logging can create usable filenames. @minggangw Do you think if we should workaround it in rclnodejs or try to fix it in rcl? rcl is using program_invocation_name which is a glibc extension, I don't know if this affects windows or mac as it uses different ways to get the program name.

@minggangw
Copy link
Member

Do you think if we should workaround it in rclnodejs or try to fix it in rcl?

As the usage of Electron is specific to rclnodejs only, I think it's fine to solve it inside rclnodejs itself.

rcl is using program_invocation_name which is a glibc extension

So, how rcl deals with the log file's name on Windows/macOS? What happens if we are running Election with rclnodejs on Windows when logging, will still meet ENAMETOOLONG?

@koonpeng
Copy link
Collaborator Author

koonpeng commented Aug 5, 2020

https://github.com/chromium/chromium/blob/f18e79d901f56154f80eea1e2218544285e62623/services/service_manager/embedder/set_process_title.cc#L88 It says here that windows and mac doesn't do anything so I believe that this only affects linux.

@minggangw
Copy link
Member

I see, so I reckon we will not have this kind of problem on Windows/macOS and this PR looks good now 👍

@minggangw minggangw self-requested a review August 5, 2020 08:10
Copy link
Member

@minggangw minggangw left a comment

Choose a reason for hiding this comment

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

Thanks for working on this issue, lgtm and thanks!

@minggangw
Copy link
Member

@koonpeng would you please change the status to “Ready for review”, so I can merge this PR.

@koonpeng koonpeng marked this pull request as ready for review August 5, 2020 09:08
@koonpeng
Copy link
Collaborator Author

koonpeng commented Aug 5, 2020

I just updated to status. Thanks!

@minggangw minggangw merged commit 5c7446d into develop Aug 5, 2020
minggangw pushed a commit that referenced this pull request Aug 7, 2020
* workaround process name mangling by chromium

rcl logging uses `program_invocation_name` to determine the log file,
chromium mangles the program name to include all args, this causes a
ENAMETOOLONG error when starting ros. Workaround is to replace the first
occurence of ' -' with the null terminator. see:
https://unix.stackexchange.com/questions/432419/unexpected-non-null-encoding-of-proc-pid-cmdline

* fix gcc warning

* only enable workaround for linux and glibc
@minggangw minggangw added this to Done in v0.15.2 Release Aug 7, 2020
@minggangw minggangw deleted the fix/electron branch September 3, 2020 14:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

3 participants