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

Print out warning only if platform is not Ubuntu and make build directory relative to where build starts #382

Closed
wants to merge 5 commits into from

Conversation

linh2931
Copy link
Member

#363 reports pinned_build.sh always prints "Currently only supporting Ubuntu based builds. Proceed at your own risk.", even on Ubuntu platform.

#364 reports the build directory is made relative to the dependency directory; it should have been relative to the directory where the script starts.

This PR fixes those two issues.

@@ -2,7 +2,7 @@

echo "Mandel Pinned Build"

if [[ $NAME != "Ubuntu" ]]
if [[ $(uname -v) != *Ubuntu* ]]
Copy link
Member

Choose a reason for hiding this comment

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

fwiw this may not yield expected behavior when running ubuntu in a container. Also, how safe is it for uname -v to not be in quotes here. Locally I see spaces in that

$ uname -v
#1 SMP PREEMPT_DYNAMIC Thu, 09 Jun 2022 16:14:10 +0000

Choose a reason for hiding this comment

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

I was expecting something more like:

#!/bin/sh

if [ -e /etc/os-release ]
then
        . /etc/os-release
fi

echo $ID

Copy link
Member Author

@linh2931 linh2931 Jun 14, 2022

Choose a reason for hiding this comment

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

Thanks Matt. I will add quotes there.

I was testing inside a Ubuntu container built on VirtualBox on my PC

uname -v
#49~20.04.1-Ubuntu SMP Wed May 18 18:44:28 UTC 2022

I will change to do uname -a to be safe.

Copy link

@matthewdarwin matthewdarwin Jun 14, 2022

Choose a reason for hiding this comment

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

This will not work. I am running a Ubuntu lxc container on a debian host. uname -a shows Debian. It is not debian (only the kernel is Debian).

image

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks @matthewdarwin. I have changed to use os-release and provide specific reason when a build is not support.

@spoonincode
Copy link
Member

This looks good but we'll want the change in 3.1 so based on our branch strategy this change needs to be PRed to the 3.1 branch first. (sorry for the unfortunate timing!) You'll need to open a new PR with these changes based on the 3.1 release branch.

@linh2931
Copy link
Member Author

Do release/3.1.x PR #487 first. And then merge that to main. Close this one.

@linh2931 linh2931 closed this Jun 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants