Install system dependencies in swiflty-install.sh #54
Conversation
|
@swift-server-bot test install please |
|
@swift-server-bot test install please |
|
@swift-server-bot test install please |
| @@ -48,6 +51,47 @@ read_input_with_default () { | |||
| fi | |||
| } | |||
|
|
|||
| yn_prompt () { | |||
There was a problem hiding this comment.
I originally thought we needed another y/n prompt, so factored it out into these functions. We didn't end up needing it, but it still seemed useful so I left it in.
| return | ||
| fi | ||
|
|
||
| dockerfile_url="https://raw.githubusercontent.com/apple/swift-docker/main/nightly-main/$docker_platform_name/$docker_platform_version/Dockerfile" |
There was a problem hiding this comment.
The system dependencies can change on each release, so I decided to just use the latest set of them to ensure a user can always install the latest swift version successfully. I thought about unioning the oldest releases' dependencies with the newest one's, but that seemed overly complex for an uncommon case.
| beg_line_num=$(printf "$dockerfile" | grep -n --max-count=1 "$package_manager.*install" | cut -d ":" -f1) | ||
|
|
||
| # Starting from there, find the first line that doesn't have the same level of indentation. | ||
| relative_end_line_num=$(printf "$dockerfile" | |
There was a problem hiding this comment.
I started using lowercase variable names for non-environment variables per https://stackoverflow.com/a/673940/16745421. In a later commit, I'll go back and update the existing variables to match this.
| apt-get remove -y zlib1g-dev | ||
| elif has_command yum ; then | ||
| yum remove -y libcurl-devel |
There was a problem hiding this comment.
These are just arbitrary packages from the dependency lists used to check if the whole set have been installed or not.
install/swiftly-install.sh
Outdated
| echo "Warning: sudo not installed and current user is not root, skipping system dependency installation." | ||
| return | ||
| elif ! has_command "$package_manager" ; then | ||
| echo "Warning: package manager \"$package_manager\" not found, skipping system dependency instllation." |
There was a problem hiding this comment.
| echo "Warning: package manager \"$package_manager\" not found, skipping system dependency instllation." | |
| echo "Warning: package manager \"$package_manager\" not found, skipping system dependency installation." |
install/swiftly-install.sh
Outdated
| # Find the line number of the RUN command associated with installing system dependencies. | ||
| beg_line_num=$(printf "$dockerfile" | grep -n --max-count=1 "$package_manager.*install" | cut -d ":" -f1) | ||
|
|
||
| # Starting from there, find the first line that doesn't have the same level of indentation. |
There was a problem hiding this comment.
Indentation seems a little flaky way to catch the full list of installation dependencies. Why don't you use the fact that the last non-whitespace character is a backslash. Maybe once you have found the install line concatenate all the lines ending with \ together. Then split the line into separate commands ie with && as separator. Then choose the install command and read the packages from it.
Its still not ideal and just hardcoding the required packages might just be preferable.
If you are going to parse the Docker file then I'd also add tests for the parsing.
There was a problem hiding this comment.
Yeah, indentation might not be be the best thing to rely on. Updated to do something more along the lines of your suggestion. I don't really want to hardcode any lists of dependencies since they vary from platform to platform and are updated every now and then.
The tests (in particular default-install.sh) do cover the parsing behavior, though not as comprehensively as they should. I've updated default-install.sh to verify more of the required dependencies were installed. For that I just hardcoded the lists, which is okay since they're just tests. We may need to update it from time to time, but the cost of doing so shouldn't be too bad.
There was a problem hiding this comment.
My regex, linux commands knowledge is a little rusty so I'm gonna have to trust you on this. But much prefer checking for \ than the tabbing.
| # Disable errexit since failing to install system dependencies is not swiftly installation-fatal. | ||
| set +o errexit | ||
| if [[ "$(id --user)" == "0" ]]; then | ||
| "$package_manager" install "${install_args[@]}" "${package_list[@]}" |
There was a problem hiding this comment.
Is there a chance on ubuntu that you need to call apt-get update first?
There was a problem hiding this comment.
Yeah, especially for a fresh docker image. That said, I think it would be better for users to do that outside of the script, since in most cases calling update shouldn't be needed, and we don't want to do it unnecessarily since it can involve downloading a decent amount of data.
There was a problem hiding this comment.
If a package install fails is it worthwhile pointing out to the user they might want to run apt-get update
There was a problem hiding this comment.
good idea, done
|
@swift-server-bot test install please |
|
@etcwilde @MaxDesiatov This might be of interested to our recent discussion in swiftlang/swift#66398 as well. |
| # Disable errexit since failing to install system dependencies is not swiftly installation-fatal. | ||
| set +o errexit | ||
| if [[ "$(id --user)" == "0" ]]; then | ||
| "$package_manager" install "${install_args[@]}" "${package_list[@]}" |
There was a problem hiding this comment.
If a package install fails is it worthwhile pointing out to the user they might want to run apt-get update
install/swiftly-install.sh
Outdated
| # Find the line number of the RUN command associated with installing system dependencies. | ||
| beg_line_num=$(printf "$dockerfile" | grep -n --max-count=1 "$package_manager.*install" | cut -d ":" -f1) | ||
|
|
||
| # Starting from there, find the first line that doesn't have the same level of indentation. |
There was a problem hiding this comment.
My regex, linux commands knowledge is a little rusty so I'm gonna have to trust you on this. But much prefer checking for \ than the tabbing.
|
@swift-server-bot test install please |
|
@swift-server-bot test install please |
Closes #32
This works by scraping the dependency list from the dockerfiles at https://github.com/apple/swift-docker. Long-term, we'll want to get the list of dependencies from a swift.org API.
The design originally called for us to not do this due to different Swift releases having different system dependencies, but upon further reflection, this seems like a big enough ergonomic win that we might as well do it.