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.

Sign up for GitHub

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

Jump to bottom

[theiaext] Suspicious PATH concatenation in theiaext #3738

Closed
kittaakos opened this issue Dec 5, 2018 · 2 comments
Closed

[theiaext] Suspicious PATH concatenation in theiaext #3738

kittaakos opened this issue Dec 5, 2018 · 2 comments
Labels
quality issues related to code and application quality question user / developer questions

Comments

@kittaakos
Copy link
Contributor

First off, I do not think the current logic can work on Windows.
https://github.com/theia-ide/theia/blob/14078b4e3c1e783ea938c641fb5694ca6c60ec11/dev-packages/ext-scripts/theiaext#L40

The delimiter is ; instead of : on Windows. We should use path.delimiter instead.
Secondly, the additional path (on macOS) does not seem good either: /Users/akos.kitta/git/theia/packages/core/../../node_modules/.bin We should normalize it.

Finally, we have to escape the path. Otherwise, it will not work with whitespaces.

Any thoughts on this, @akosyakov? If we do not use this additional PATH entry, maybe we can remove it. I can make the required changes if this is accepted as a bug.
@kittaakos kittaakos added question user / developer questions quality issues related to code and application quality labels Dec 5, 2018
@kittaakos
Copy link
Contributor Author

Maybe it is a leftover before we switched from npm to yarn. Currently, theiaext is in the node_modules folder of the caller, so we do not have to add the root node_modules/.bin to the PATH.

kittaakos pushed a commit that referenced this issue Dec 5, 2018
GH-3738: Cleaned up theiaext.
bab89af 
 - Removed the flawed `PATH` entry concatenation. It is unused.
 - Fixed the linter errors.
 - Logged the `err` object with the stack trace if possible.

Signed-off-by: Akos Kitta <kittaakos@typefox.io>
kittaakos pushed a commit that referenced this issue Dec 5, 2018
GH-3738: Added the normalized, escaped PATH entry to the PATH.
52d606d 
Closes: #3738.
Signed-off-by: Akos Kitta <kittaakos@typefox.io>
kittaakos pushed a commit that referenced this issue Dec 5, 2018
GH-3738: Cleaned up theiaext.
7e1c4bf 
 - Removed the flawed `PATH` entry concatenation. It is unused.
 - Use a shallow copy of the `env`.
 - Fixed the linter errors.
 - Logged the `err` object with the stack trace if possible.

Signed-off-by: Akos Kitta <kittaakos@typefox.io>
kittaakos pushed a commit that referenced this issue Dec 6, 2018
@kittaakos
GH-3738: Cleaned up theiaext.
1a45599 
 - Removed the flawed `PATH` entry concatenation. It is unused.
 - Use a shallow copy of the `env`.
 - Fixed the linter errors.
 - Logged the `err` object with the stack trace if possible.

Signed-off-by: Akos Kitta <kittaakos@typefox.io>
@kittaakos
Copy link
Contributor Author

Closed via #3741.

bogthe pushed a commit to ARMmbed/theia that referenced this issue Jan 21, 2019
eclipse-theiaGH-3738: Cleaned up theiaext.
c2bd443 
 - Removed the flawed `PATH` entry concatenation. It is unused.
 - Use a shallow copy of the `env`.
 - Fixed the linter errors.
 - Logged the `err` object with the stack trace if possible.

Signed-off-by: Akos Kitta <kittaakos@typefox.io>
Signed-off-by: Bogdan Stolojan <petre.stolojan@arm.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
quality issues related to code and application quality question user / developer questions
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
1 participant
@kittaakos