fix(webserver): better log-stderr.sh reporting, remove trixie-backports#8042
fix(webserver): better log-stderr.sh reporting, remove trixie-backports#8042
Conversation
|
Download the artifacts for this pull request:
See Testing a PR. |
a004c7f to
505059e
Compare
tyler36
left a comment
There was a problem hiding this comment.
Does what the "manual testing" claims.
I tested with current stable v1.24.10 so some output did not match with expected "ddev head".
However, all "This PR" (v1.24.10-135-gb7dfb1079) output matched.
| # Currently used in 2 places: n-install.sh, install_nvm.sh | ||
| - START_SCRIPT_TIMEOUT={{ div (sub .DefaultContainerTimeout 5) 2 }} | ||
| # Currently used in 1 place: n-install.sh | ||
| - START_SCRIPT_TIMEOUT={{ div (sub .DefaultContainerTimeout 5) 1 }} |
There was a problem hiding this comment.
| - START_SCRIPT_TIMEOUT={{ div (sub .DefaultContainerTimeout 5) 1 }} | |
| - START_SCRIPT_TIMEOUT={{ sub .DefaultContainerTimeout 5 }} |
What's the reasoning behind diving by 1 ((120 - 5) / 1 = 115)? It seems unnessasary from a maths point of view.
There was a problem hiding this comment.
I left it here for better understanding what needs to be done, if we add some new extra script.
I can't remember syntax like this {{ div (sub .DefaultContainerTimeout X) Y }}
I'll add an example for it in the comments.
b7dfb10 to
82f735a
Compare
| ```bash | ||
| ddev exec "curl -LsS https://download.civicrm.org/latest/civicrm-STABLE-standalone.tar.gz -o /tmp/civicrm-standalone.tar.gz" | ||
| ddev exec "tar --strip-components=1 -xzf /tmp/civicrm-standalone.tar.gz" | ||
| ddev composer update civicrm/composer-compile-plugin --no-scripts |
There was a problem hiding this comment.
This is not needed anymore, because CiviCRM stable release already has the newer civicrm/composer-compile-plugin.
82f735a to
8c73ee7
Compare
|
In HEAD, I see the problem (nodejs version not set) but I don't see anything like this in I doubt that matters, because this PR will fix the problem, but I didn't understand the OP completely apparently. |
rfay
left a comment
There was a problem hiding this comment.
It's great, thank you!
Could you please add a whole usage guide to the top of log-stderr.sh ? It seems like there are correct ways to use it and different usages in different contexts.
| exit_code=$? | ||
|
|
||
| # Wait for background process substitution (tee) to complete writing to file | ||
| wait |
There was a problem hiding this comment.
WOW. I had no idea you could do this.
There was a problem hiding this comment.
In HEAD, I see the problem (nodejs version not set) but I don't see anything like this in
ddev logs:+ n-install.sh This script cannot write to the directory /usr/local/binI doubt that matters, because this PR will fix the problem, but I didn't understand the OP completely apparently.
I think this is why you didn't see anything in the logs, because there was no wait.
| # Run "apt-get update" manually only for mariadb and debian repos to make it faster | ||
| log-stderr.sh apt-get update -o Acquire::Retries=5 -o Dir::Etc::sourcelist="sources.list.d/mariadb-archive.sources" -o Dir::Etc::sourceparts="-" -o APT::Get::List-Cleanup="0" || exit $? | ||
| log-stderr.sh apt-get update -o Acquire::Retries=5 -o Dir::Etc::sourcelist="sources.list.d/debian.sources" -o Dir::Etc::sourceparts="-" -o APT::Get::List-Cleanup="0" || exit $? | ||
| apt-get update -o Acquire::Retries=5 -o Dir::Etc::sourcelist="sources.list.d/mariadb-archive.sources" -o Dir::Etc::sourceparts="-" -o APT::Get::List-Cleanup="0" |
There was a problem hiding this comment.
I didn't understand this change. Why does this no longer have log-stderr.sh ?
There was a problem hiding this comment.
I moved log-stderr.sh outside the script:
if slices.Contains([]string{nodeps.MariaDB1011, nodeps.MariaDB114}, app.Database.Version) {
extraWebContent = extraWebContent + "\nRUN log-stderr.sh mariadb-client-install.sh || true\n"
}
Added in I didn't rebuild |
|
Thank you for adding usage, and there's no need for rebuild. It does seem like there are two (or three?) different contexts for log-stderr.sh though
Is that true? If so maybe the comment can include information about how those contexts should be used or managed differently. |
|
Updated it. |
|
Thanks! |
The Issue
n-install.shdoesn't work if permissions on/usr/local/binare changed:There are two problems here:
COPY --fromddev startabout Node.js not being installed correctly.I had this problem in:
If people want to install
libcurl4-openssl-devin DDEV HEAD:The correct command will be:
Or:
I don't see any benefit in using
trixie-backports, because it's disabled by default, and you need to add*/trixie-backportssuffix for all dependencies or provide a-t trixie-backportsflag. It turns out it's more complicated than I thought.How This PR Solves The Issue
curl/trixie-backportswithcurl, because this change was going to break configuration for people who installlibcurl-devor any other dependencies oncurl. I introduced this change in build(curl): use trixie-backports to install newer curl #7897, but it doesn't seem to be a good idea to use it.trixie-backportsrepository, people can install it manually if the want to, and this should be a part of the blog article Blog: All you need to know about installing software in DDEV ddev.com#383trixie-backportsexclusively for the CiviCRM test, it doesn't fail locally, but always fails in GitHub Actionslog-stderr.shoutside ofn-install.sh,mariadb-client-install.sh,mysql-client-install.sh- any errors should be reported, not only network problemslog-stderr.shtomariadb-compat-install.shlog-stderr.shby addingwaitSTART_SCRIPT_TIMEOUTvalue, because we don't haveinstall_nvm.shanymore, see chore(deprecation): remove ddev nvm functionality in v1.25.0 #7826chmod -f ugo+rwx /usr/local/bin /usr/local/bin/*as one of the last commands in.ddev/.webimageBuild/DockerfileManual Testing Instructions
Check permissions on
/usr/local/bin:DDEV HEAD:
This PR:
Test
n-install.shfailure:DDEV HEAD:
This PR:
Test
libcurl4-openssl-devinstallation:DDEV HEAD:
This PR:
START_SCRIPT_TIMEOUTshould be increased, because we don't haveinstall_nvm.shanymore:DDEV HEAD, formula: (120 - 5) / 2 = 57
This PR, formula: 120 - 5 = 115
Automated Testing Overview
Release/Deployment Notes