-
Notifications
You must be signed in to change notification settings - Fork 1.5k
[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
Conversation
@alexrallen, could you please take a look? Thanks. |
There was a problem hiding this 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.
@alexrallen, thanks for your comment, I will change it. Do you know what value should be set for SONIC_ROOT? |
For %%SONIC_ROOT%%, looks like it is not used at all, I do not fix SONIC_ROOT in this PR. |
@alexrallen, could you please help review it again? Thanks. |
28df14a
to
34873dd
Compare
@dgsudharsan, could you please take a look the PR? |
There was a problem hiding this 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%% |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
How I did it
How to verify it
Which release branch to backport (provide reason below if selected)
Description for the changelog
Link to config_db schema for YANG module changes
A picture of a cute animal (not mandatory but encouraged)