Skip to content

Fix cache related mellanox bullseye build failures #9234

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Nov 16, 2021
Merged

Fix cache related mellanox bullseye build failures #9234

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions platform/mellanox/issu-version.dep
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@ DEP_FILES := $(SONIC_COMMON_FILES_LIST) $(PLATFORM_PATH)/issu-version.mk $(PLATF
DEP_FILES += $(SONIC_COMMON_BASE_FILES_LIST)
DEP_FILES += $(shell git ls-files -- $(SPATH))

#$(ISSU_VERSION_FILE)_CACHE_MODE := GIT_CONTENT_SHA
$(ISSU_VERSION_FILE)_CACHE_MODE := none
$(ISSU_VERSION_FILE)_CACHE_MODE := GIT_CONTENT_SHA
$(ISSU_VERSION_FILE)_DEP_FLAGS := $(SONIC_COMMON_FLAGS_LIST)
$(ISSU_VERSION_FILE)_DEP_FILES := $(DEP_FILES)
6 changes: 6 additions & 0 deletions platform/mellanox/issu-version.mk
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,13 @@

ISSU_VERSION_FILE = issu-version
$(ISSU_VERSION_FILE)_SRC_PATH = $(PLATFORM_PATH)/issu-version

ifeq ($(BLDENV), buster)
Copy link
Collaborator

@qiluo-msft qiluo-msft Nov 15, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

buster

Wondering what is the DEPENDS for debian version before buster and after buster? #Closed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can you please elaborate on your question?

This line exists because this only depends on APPLIBS when the build environment is buster (when it is actually generating the issu_version file)

Otherwise it will try to compile the mellanox sdk for bullseye (which it is not ready to do right now)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Just to double check: you mean issu_version only exists in Buster, Not in Bulleyes and later, not in Stretch and earlier versions?

$(ISSU_VERSION_FILE)_DEPENDS += $(APPLIBS)
endif

SONIC_MAKE_FILES += $(ISSU_VERSION_FILE)

MLNX_FILES += $(ISSU_VERSION_FILE)

export ISSU_VERSION_FILE
7 changes: 5 additions & 2 deletions platform/mellanox/issu-version/Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,12 +19,15 @@ SHELL = /bin/bash
.SHELLFLAGS += -e

MAIN_TARGET = issu-version
BULLSEYE_FILES_PATH = /sonic/target/files/bullseye
BUSTER_FILES_PATH = /sonic/target/files/buster

$(addprefix $(DEST)/, $(MAIN_TARGET)): $(DEST)/% :
ifneq (,$(findstring buster,$(DEST)))
Copy link
Collaborator

@qiluo-msft qiluo-msft Nov 15, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Why this rule depends on Debian version? #Pending

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not on the debian version but what is currently being built.

If buster is being built it executes the program which exports the contents of the issue_version file in the buster slave docker.

If bullseye is being built it doesn't have access to this utility so it copies from the buster targets (which has already been built) to itself.

We do this so that we have the file available in both the buster and bullseye slave docker at build time and that both targets are correctly recognized by the build system and cached correctly.

Copy link
Collaborator

Choose a reason for hiding this comment

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

What is the reason of "If bullseye is being built it doesn't have access to this utility"? Any plan to have the utility (what is it?) in future?

I am wondering if you can make this Makefile not depends on any Debian versions, it should work for Buster, and any future Debian versions.

Copy link
Collaborator

Choose a reason for hiding this comment

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

You assume build order of Buster/Bullseye. Could you be explicit. I understand this PR is a temporary solution, and after SDK support Bullseye, all the Debian version dependency will be removed.

sx_sdk --version | egrep -o 'ISSU [0-9]+' > $(DEST)/$(MAIN_TARGET) || {
echo "ISSU version is undefined: $(sx_sdk --version)"
rm $(DEST)/$(MAIN_TARGET)
exit 1
}
cp $(DEST)/$(MAIN_TARGET) $(BULLSEYE_FILES_PATH)/$(MAIN_TARGET)
else
cp $(BUSTER_FILES_PATH)/$(MAIN_TARGET) $(DEST)/$(MAIN_TARGET)
endif
2 changes: 1 addition & 1 deletion platform/mellanox/mft/Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ $(addprefix $(DEST)/, $(MAIN_TARGET)): $(DEST)/% :
wget -O $(MFT_TGZ) http://www.mellanox.com/downloads/MFT/$(MFT_TGZ)
tar xzf $(MFT_TGZ)

ifeq ($(BLDENV), bullseye)
ifneq (,$(findstring bullseye,$(DEST)))
Copy link
Collaborator

@qiluo-msft qiluo-msft Nov 15, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Could you explain this change? #Pending

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, this checks if the target is bullseye or buster by looking at the destination path for the target.

Copy link
Collaborator

Choose a reason for hiding this comment

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

your code is only check bullseye? How it is different with original code?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

BLDENV is not exported to Makefiles, thus this was not functioning.

Copy link
Collaborator

Choose a reason for hiding this comment

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

However I notice is largely used by many other makefile. Is it something missing in build infra?

pushd $(MFT_NAME)/SDEBS

# put a lock here because dpkg does not allow installing packages in parallel
Expand Down