Skip to content
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:toolchain:Remove privileged operation from build script #839

Merged
merged 4 commits into from
Aug 22, 2019

Conversation

mvglasow
Copy link
Contributor

As mentioned in #827, this PR moves privileged operations (sudo apt-get install) out of the build script, replacing it with a check if the required component is installed (if it is not, the script will abort with an error).

For local builds, you need to install ant yourself. If you haven’t done so before starting the build, the script will abort with a meaningful error message.

For CircleCI, ant has been added to the list of packages to install.

On F-Droid this will be a separate merge request (Link will follow).

mvglasow added 2 commits August 21, 2019 23:46
@mvglasow mvglasow requested a review from pgrandin August 21, 2019 21:43
@mvglasow
Copy link
Contributor Author

@jandegr could you take a look at this as well, just to make sure I am not inadvertently breaking anything?

@mvglasow
Copy link
Contributor Author

And the related F-Droid merge request is at https://gitlab.com/fdroid/fdroiddata/merge_requests/5289

@pgrandin
Copy link
Contributor

I think that it makes sense 👍

Copy link
Member

@jkoan jkoan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perfect!
Personally I like the the longer if else construct more, but it is perfectly fine how it is now

Copy link
Contributor

@lains lains left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops, I hadn't seen @jkoan's comment that is probably exactly saying the same thing...


sudo apt-get install -y ant
which ant > /dev/null || { echo "FATAL: ant is not installed; install manually and retry."; exit 1; }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe this line good be written in a way that is more compatible with generic shell syntax like:

if which ant >/dev/null; then
    echo "FATAL: ant is not installed; install manually and retry." >&2
    exit 1
fi

Copy link
Contributor Author

@mvglasow mvglasow Aug 22, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will fail if ant is found and succeed if it is not found (just tried it). I am not a shell guru, though; all I know is that anything involving shell scripts and success/failure/true/false/if is highly confusing.

Redirecting to stderr is a good point, though.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mmm... OK, I missed the fact that there is an issue because of the mix of if then construct and set -e

Signed-off-by: mvglasow <michael -at- vonglasow.com>
@mvglasow mvglasow merged commit 5e644da into navit-gps:trunk Aug 22, 2019
viktorgino pushed a commit that referenced this pull request Sep 22, 2020
 Fix:toolchain:Remove privileged operation from build script
viktorgino pushed a commit that referenced this pull request Sep 22, 2020
 Fix:toolchain:Remove privileged operation from build script
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants