-
-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Add config to do desktop notifications by running an abitrary command. #15013
Conversation
def notify_with_command(command): | ||
def notify(title, text): | ||
import subprocess | ||
subprocess.check_call([command, title, text]) |
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 should be replaced with the call
wrapper imported from command_base
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.
Done.
|
||
|
||
def notify(title, text): | ||
def notify(config, title, text): | ||
"""Generate a desktop notification using appropriate means on | ||
supported platforms Linux, Windows, and Mac OS. On unsupported | ||
platforms, this function acts as a no-op.""" |
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.
Please update this to say that we can also add a custom command to the config for generating a notification.
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.
Done.
6960a2d
to
5168a69
Compare
@bors-servo r+ |
📌 Commit 5168a69 has been approved by |
Add config to do desktop notifications by running an abitrary command. <!-- Please describe your changes on the following line: --> I now have an usual setup where I use a (fast) remote machine for building, and I’m adding a hack to forward end-of-build notifications back to my laptop. This is the motivation for this change, but I kept it general enough that it could be used in other situations. --- <!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: --> - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [ ] These changes fix #__ (github issue number if applicable). <!-- Either: --> - [ ] There are tests for these changes OR - [ ] These changes do not require tests because _____ <!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. --> <!-- Reviewable:start --> --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/15013) <!-- Reviewable:end -->
Please address it if you don't mind. If you feel there's a good reason to not do that change, feel free to r=me after commenting on why. @bors-servo r- |
5168a69
to
59ee5df
Compare
59ee5df
to
16d8c35
Compare
@bors-servo r=emilio |
📌 Commit 16d8c35 has been approved by |
Add config to do desktop notifications by running an abitrary command. <!-- Please describe your changes on the following line: --> I now have an usual setup where I use a (fast) remote machine for building, and I’m adding a hack to forward end-of-build notifications back to my laptop. This is the motivation for this change, but I kept it general enough that it could be used in other situations. --- <!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: --> - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [ ] These changes fix #__ (github issue number if applicable). <!-- Either: --> - [ ] There are tests for these changes OR - [ ] These changes do not require tests because _____ <!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. --> <!-- Reviewable:start --> --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/15013) <!-- Reviewable:end -->
☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css, mac-rel-wpt1, mac-rel-wpt2, windows-gnu-dev, windows-msvc-dev |
I now have an usual setup where I use a (fast) remote machine for building, and I’m adding a hack to forward end-of-build notifications back to my laptop. This is the motivation for this change, but I kept it general enough that it could be used in other situations.
./mach build -d
does not report any errors./mach test-tidy
does not report any errorsThis change is