Skip to content

[Bug][Build] Fix the variable patterns not replaced issue#11572

Merged
qiluo-msft merged 3 commits intosonic-net:masterfrom
xumia:fix-build-script-issue
Aug 2, 2022
Merged

[Bug][Build] Fix the variable patterns not replaced issue#11572
qiluo-msft merged 3 commits intosonic-net:masterfrom
xumia:fix-build-script-issue

Conversation

@xumia
Copy link
Copy Markdown
Collaborator

@xumia xumia commented Jul 28, 2022

Why I did it

The %%EXTRA_CMDLINE_LINUX%% is not replaced to the real value, it has impact on the kernel parameter settings.
See the log sonic-vs.img.gz.log in the latest master build. In the grub.cfg, the %%EXTRA_CMDLINE_LINUX%% is set in the linux command line.

Installing for i386-pc platform.
Installation finished. No error reported.
Switch CPU vendor is: GenuineIntel
Switch CPU cstates are: disabled
EXTRA_CMDLINE_LINUX=%%EXTRA_CMDLINE_LINUX%%
Installed SONiC base image SONiC-OS successfully
ONIE: NOS install successful: file://dev/vdb/onie-installer.bin

How I did it

How to verify it

Which release branch to backport (provide reason below if selected)

  • 201811
  • 201911
  • 202006
  • 202012
  • 202106
  • 202111
  • 202205

Description for the changelog

Link to config_db schema for YANG module changes

A picture of a cute animal (not mandatory but encouraged)

@xumia xumia requested a review from lguohan as a code owner July 28, 2022 14:36
@xumia
Copy link
Copy Markdown
Collaborator Author

xumia commented Jul 28, 2022

@alexrallen, could you please take a look? Thanks.

@xumia xumia requested a review from qiluo-msft July 28, 2022 14:45
Copy link
Copy Markdown
Contributor

@alexrallen alexrallen left a comment

Choose a reason for hiding this comment

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

It looks like when I refactored some of this code in install.sh outside of that file I failed to consider this find/replace operation we were performing. Thanks for catching this.

I would prefer from an cleanliness perspective to reduce the amount of sed processing that we do here.

I suggest we setup the setup the variables in install.sh and then default_installer.conf and any scripts called by the installer will inherit variables without having to add this regex code to onie-mk each time (and prevent bugs like this in the future)

An example of this would be removing the following line from default_platform.conf:
https://github.com/Azure/sonic-buildimage/blob/master/installer/default_platform.conf#L474
So that this line in install.sh is able to pass the value down to default_platform.conf
https://github.com/Azure/sonic-buildimage/blob/master/installer/install.sh#L239

In addition it seems there are three instances of having these templates in default_platform.conf
https://github.com/Azure/sonic-buildimage/blob/master/installer/default_platform.conf#L474
https://github.com/Azure/sonic-buildimage/blob/master/installer/default_platform.conf#L463
https://github.com/Azure/sonic-buildimage/blob/master/installer/default_platform.conf#L67

We need to make sure all three of these instances are handled (I don't think the %%SONIC_ROOT%% is handled in this patch.

Let me know if there are any questions.

@xumia
Copy link
Copy Markdown
Collaborator Author

xumia commented Jul 28, 2022

SONIC_ROOT

@alexrallen, thanks for your comment, I will change it. Do you know what value should be set for SONIC_ROOT?
(FYI. I have another PR for FIPS pending on the EXTRA_CMDLINE_LINUX, so I sent the PR.)

@xumia
Copy link
Copy Markdown
Collaborator Author

xumia commented Jul 29, 2022

For %%SONIC_ROOT%%, looks like it is not used at all, I do not fix SONIC_ROOT in this PR.
We support 4 image types: onie, raw, kvm, aboot. The first 3 image types are use onie (calling generate_onie_installer_image), aboot use files/Aboot/boot0.j2, not relative to install.sh.
@alexrallen, do you think it is good or not?

@xumia
Copy link
Copy Markdown
Collaborator Author

xumia commented Jul 31, 2022

@alexrallen, could you please help review it again? Thanks.

@xumia xumia force-pushed the fix-build-script-issue branch from 28df14a to 34873dd Compare July 31, 2022 22:45
@xumia
Copy link
Copy Markdown
Collaborator Author

xumia commented Aug 1, 2022

@dgsudharsan, could you please take a look the PR?

Copy link
Copy Markdown
Contributor

@alexrallen alexrallen left a comment

Choose a reason for hiding this comment

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

This looks fine to me. Thanks @xumia

fi

# Add extra linux command line
extra_cmdline_linux=%%EXTRA_CMDLINE_LINUX%%
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

EXTRA_CMDLINE_LINUX

The 3 lines were added in one commit, it makes sense to remove all 3 lines together.

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.

3 participants