Fix the cronjob script in the remoting Debian package. [chromium/src : main]
Lambros Lambrou (Gerrit)
Lambros Lambrou voted and added 1 comment![]( "Open in Gerrit")
Votes added by Lambros Lambrou
| Auto-Submit | +1 |
1 comment
yuweih@: please review
thomasanderson@: FYI, maybe there's an alternative way of fixing this?
Related details
- Yuwei Huang
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Yuwei Huang (Gerrit)
Yuwei Huang voted![]( "Open in Gerrit")
- Lambros Lambrou
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
Fix the cronjob script in the remoting Debian package.
The remoting Debian package includes a cronjob script, which is used
to install an APT repository source and add a Google signing-key to APT.
This file is generated using the same templates that Chrome uses for
its Debian packaging. A recent refactoring changed Chrome's
template-processor to use a Python library, but it left the remoting
builder using the old Bash-based implementation.
This CL fixes remoting's processor to work with Chrome's updated
templates. Care is taken to correctly handle the substitutions
`@@repoconfigregex` and `@@repoconfig`, because one is a substring
of the other. This wasn't a problem with the previous template language
because it used the `@@foo@@` syntax.
This is a short-term fix intended for merging onto M145 branch. In
the longer term, remoting should use the same template-processor as
Chrome.
- M remoting/host/installer/linux/build-deb.sh
- M remoting/host/installer/linux/debian/repo.cron
- M remoting/host/installer/linux/installer.include
Code-Review: +1 by Yuwei Huang
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Thomas Anderson (Gerrit)
Thomas Anderson added 1 comment![]( "Open in Gerrit")
yuweih@: please review
thomasanderson@: FYI, maybe there's an alternative way of fixing this?
IIUC, the issue is caused by common/variables.include missing from //remoting/host/installer/linux.
The fix is not for //remoting to use chrome/installer/linux/common/variables.include. Instead, it should copy that file (pre-python changes) to //remoting, then it can also continue using the older substitution @@syntax@@.
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Lambros Lambrou (Gerrit)
Lambros Lambrou added 1 comment![]( "Open in Gerrit")
Thomas Andersonyuweih@: please review
thomasanderson@: FYI, maybe there's an alternative way of fixing this?
IIUC, the issue is caused by common/variables.include missing from //remoting/host/installer/linux.
The fix is not for //remoting to use chrome/installer/linux/common/variables.include. Instead, it should copy that file (pre-python changes) to //remoting, then it can also continue using the older substitution @@syntax@@.
Thank you for taking a look! The `common/variables.include` file is 1 line - we could have simply copied it in, as you suggest.
The main problem is `apt.include`, which generates most of the cronjob script. That pulls in the same file that Chrome uses - there is only 1 file named `apt.include` in the Chromium repository. That file uses the new `@@foo` syntax and lower-case attributes such as `@@architecture`, and it pulls in a couple of other templates, which also use the new syntax.
So our choices are:
1. Copy `apt.include` and other templates into remoting, change them all to use the old syntax, and use the old parser.
2. Update remoting to use Chrome's parser and templates. Try to share as much code as possible between Chrome and remoting package-builds.
3. Update remoting's old parser for the new syntax, and use Chrome's templates.
I went with option 3 as it resulted in a very small, mergable CL.
Option 1 would make remoting's package build more independent from Chrome. We would have full flexibility to make changes independently, but we would not benefit from any bug-fixes and improvements to Chrome's packaging. Eventually, we could replace the Bash-based parser with something else.
Option 2 seems better because of the code sharing. But it might be a lot more work. I feel that, if it were simple, you would have done this already as part of your refactoring.
I'm not very familiar with Chrome's Debian packaging, so I don't have a good feel for how much code should be shared between Chrome and remoting.
Also, there is something weird going on with the 2 `@@include@@` statements in our `repo.cron`. The second one seems to resolve relative to where the first one resolved. That seems like a bug in the old template-parser. To keep this CL small, I didn't try to fix this.
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Thomas Anderson (Gerrit)
Thomas Anderson added 1 comment![]( "Open in Gerrit")
Thomas Andersonyuweih@: please review
thomasanderson@: FYI, maybe there's an alternative way of fixing this?
Lambros LambrouIIUC, the issue is caused by common/variables.include missing from //remoting/host/installer/linux.
The fix is not for //remoting to use chrome/installer/linux/common/variables.include. Instead, it should copy that file (pre-python changes) to //remoting, then it can also continue using the older substitution @@syntax@@.
Thank you for taking a look! The `common/variables.include` file is 1 line - we could have simply copied it in, as you suggest.
The main problem is `apt.include`, which generates most of the cronjob script. That pulls in the same file that Chrome uses - there is only 1 file named `apt.include` in the Chromium repository. That file uses the new `@@foo` syntax and lower-case attributes such as `@@architecture`, and it pulls in a couple of other templates, which also use the new syntax.
So our choices are:
1. Copy `apt.include` and other templates into remoting, change them all to use the old syntax, and use the old parser.
2. Update remoting to use Chrome's parser and templates. Try to share as much code as possible between Chrome and remoting package-builds.
3. Update remoting's old parser for the new syntax, and use Chrome's templates.I went with option 3 as it resulted in a very small, mergable CL.
Option 1 would make remoting's package build more independent from Chrome. We would have full flexibility to make changes independently, but we would not benefit from any bug-fixes and improvements to Chrome's packaging. Eventually, we could replace the Bash-based parser with something else.
Option 2 seems better because of the code sharing. But it might be a lot more work. I feel that, if it were simple, you would have done this already as part of your refactoring.
I'm not very familiar with Chrome's Debian packaging, so I don't have a good feel for how much code should be shared between Chrome and remoting.
Also, there is something weird going on with the 2 `@@include@@` statements in our `repo.cron`. The second one seems to resolve relative to where the first one resolved. That seems like a bug in the old template-parser. To keep this CL small, I didn't try to fix this.
I see, thank you for explaining. My recommendation is to decouple remoting from Chrome's packaging. So I agree with option 1 to copy the templates.
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |