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: do not propagate GDK_BACKEND env variable to subproc #1
base: main
Are you sure you want to change the base?
Conversation
spec-main/chromium-spec.ts
Outdated
await promisifiedTimers.setTimeout(1000); | ||
|
||
let gdkBackend = fs.readFileSync('/tmp/groot-says', 'utf8'); | ||
gdkBackend = gdkBackend.replace(/(\r\n|\n|\r)/gm, ''); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not 100% sure, but I think .trim()
will do exactly this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jviotti I see that now. Used the snippet from Shelley's PR
@@ -0,0 +1,3 @@ | |||
#!/bin/sh | |||
|
|||
echo 'I am goot!' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you mean groot
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bavulapati added some comments. can you also try checking if reverting https://chromium-review.googlesource.com/c/chromium/src/+/2586184 fixes the issue completely? It seems to be the source of the problem but I'm not sure if Weston sessions are even relevant to electron.
In the current state of the pr, changes made to process.env
doesn't get propagated to shell.openExternal()
which does not seem correct.
spec-main/chromium-spec.ts
Outdated
it('does not propagate GDK_BACKEND', async () => { | ||
const appPath = path.join(fixturesPath, 'api', 'gdk-backend-check'); | ||
|
||
console.log('gdk_backend before shell open', process.env.GDK_BACKEND); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does electron use console.log()
statements in the rest of their tests for general logging purposes? does it show up when you run the test suite or only when the specific test fails?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the console.log
statements should be removed, maybe they were just for debugging?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can remove the log as it was for debugging purposes.
@@ -0,0 +1,3 @@ | |||
#!/bin/sh | |||
echo "I'm Groot!" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why are we logging this? the test doesn't seem to validate if this has been printed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the echo statements in both scripts should be dropped.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This again is just for debugging purposes.
@@ -0,0 +1,6 @@ | |||
[Desktop Entry] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are all of these fields necessary? can we trim out some of them?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
desktop entry must have a Type
and a Name
key. Our use case needs Exec
and MimeType
.
We can remove Terminal
, as it doesn't affect our use case, not mandatory as well.
@@ -0,0 +1,17 @@ | |||
#!/bin/sh |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if the script somehow fails, it will leave the system in a mutated condition which might not be something the people who run these tests locally or on ci want. can we plz make these tests more robust by using a trap?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does having this in the afterTest help?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it wouldn't help if the process receives something like a SIGINT, which can only be handled using a trap
cp ./groot.xml ~/.local/share/mime/packages/ | ||
|
||
# update the MIME database | ||
update-mime-database ~/.local/share/mime |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can u verify if these non-trivial commands are blocking? I just wanted to make sure this doesn't cause any race condition when the rest of the test expects these operations to be completed but they are actually in progress
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From the documentation, it looks like a synchronous operation as the command can print the summary
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good! Is that applicable for these too:
- update-desktop-database
- desktop-file-install
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@RaisinTen Can you help me understand if these are blocking?
How can we confirm the same?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed the rest of the review comments and the PR is ready if we can resolve this comment
spec-main/chromium-spec.ts
Outdated
it('does not propagate GDK_BACKEND', async () => { | ||
const appPath = path.join(fixturesPath, 'api', 'gdk-backend-check'); | ||
|
||
console.log('gdk_backend before shell open', process.env.GDK_BACKEND); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the console.log
statements should be removed, maybe they were just for debugging?
@@ -0,0 +1,3 @@ | |||
#!/bin/sh | |||
echo "I'm Groot!" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the echo statements in both scripts should be dropped.
test changes are the same, so I'm fine with it |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm!
8239336
to
15af724
Compare
3bd1262
to
20f953c
Compare
42c3150
to
e5fc898
Compare
e5fc898
to
a0e5726
Compare
Description of Change
Closes electron#28436.
This PR tries to cherry-pick the commits from an open PR electron#29606 and address the review comments and add tests for the changes.
cc: @dsanders11 @RaisinTen @jviotti
Checklist
npm test
passesRelease Notes
Notes: none