Skip to content

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

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

Merged
merged 3 commits into from
Aug 2, 2022

Conversation

xumia
Copy link
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
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
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
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
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
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
Collaborator Author

xumia commented Aug 1, 2022

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

Copy link
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

@@ -471,7 +470,6 @@ EOF
fi

# Add extra linux command line
extra_cmdline_linux=%%EXTRA_CMDLINE_LINUX%%
Copy link
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