Skip to content

Conversation

@shishirb-MSFT
Copy link
Collaborator

No description provided.

@shishirb-MSFT shishirb-MSFT requested a review from a team as a code owner March 10, 2023 19:43
Copy link

@JeffMill JeffMill left a comment

Choose a reason for hiding this comment

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

looks good overall, just some suggestions added.

Is there a task on ADU to fix up the way we call DO?

installQemu
}

function isSupportedLinux()
Copy link

@JeffMill JeffMill Mar 10, 2023

Choose a reason for hiding this comment

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

all of this isSupported*, determine* stuff should be in a separate script.

You can then include those methods (I think!) using "source platform-helpers.sh" in this script. #WontFix

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good point. I'll split it out when there is another script that needs the same functionality.


echo "OS = $OS"
echo "VER = $VER"
if [ "$is_amd64" = true ]; then echo "Arch = amd64"
Copy link

@JeffMill JeffMill Mar 10, 2023

Choose a reason for hiding this comment

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

determine_os_and_arch should also return $ARCH.

(in otherwords, this code should be in determine_os_and_arch and set $ARCH) #Resolved

echo "[INFO] Installing build dependencies"

if [[ "$PLATFORM" == "osx" ]];
if [ "$is_macos" = true ];
Copy link

@JeffMill JeffMill Mar 10, 2023

Choose a reason for hiding this comment

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

rather than a bunch of "is" bools, these are mutually exclusive -- helper should set an $OSFLAVOR, e.g. "linux" e.g. if $OSTYPE == linux*
#Resolved

function isSupportedLinux()
{
if [[ "$PLATFORM" == "debian10" || "$PLATFORM" == "ubuntu1804" || "$PLATFORM" == "ubuntu2004" ]];
if [[ ("$OS" == "ubuntu" && ($VER == "18.04" || $VER == "20.04"))
Copy link

@JeffMill JeffMill Mar 10, 2023

Choose a reason for hiding this comment

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

is 18.xx or 20xx not supported? Why specifically 18.04 and 20.04 ? #Resolved

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

There was no requirement for other versions and we have not tested this on those other versions.

echo "Failed to get cpu architecture."
return 1
else
if [[ $arch == aarch64* || $arch == armv8* ]]; then
Copy link

@JeffMill JeffMill Mar 10, 2023

Choose a reason for hiding this comment

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

see my other comment -- these are mutually exclusive. e.g. $ARCH_FLAVOR="arm32" would be better IMHO #Resolved

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, good call.

source: .
override-build: |
./build/scripts/bootstrap.sh --platform ubuntu2004 --install build
./build/scripts/bootstrap.sh --install build
Copy link

@JeffMill JeffMill Mar 10, 2023

Choose a reason for hiding this comment

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

yay!!! #Resolved

@shishirb-MSFT
Copy link
Collaborator Author

shishirb-MSFT commented Mar 10, 2023

looks good overall, just some suggestions added.

Is there a task on ADU to fix up the way we call DO?


In reply to: 1335530563

Copy link
Contributor

@JeffreySaathoff JeffreySaathoff left a comment

Choose a reason for hiding this comment

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

:shipit:

@shishirb-MSFT shishirb-MSFT merged commit 62d76bc into develop Mar 11, 2023
@shishirb-MSFT shishirb-MSFT deleted the user/shishirb/active branch March 11, 2023 00:13
Copy link

@JeffMill JeffMill left a comment

Choose a reason for hiding this comment

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

awesome, thanks

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