Skip to content

Fix for sonic-config-engine build failure - test_buffers_dell6100_render_template #18566

Open
krismarvell wants to merge 1 commit intosonic-net:masterfrom
krismarvell:sonic_master_issue_18358_fix
Open

Fix for sonic-config-engine build failure - test_buffers_dell6100_render_template #18566
krismarvell wants to merge 1 commit intosonic-net:masterfrom
krismarvell:sonic_master_issue_18358_fix

Conversation

@krismarvell
Copy link
Copy Markdown
Contributor

Why I did it

To fix sonic-config-engine build error at src/sonic-config-engine/tests/test_j2files.py unit test case while using "make PLATFORM=$PLATFORM target/sonic-$PLATFORM.bin" commands. This will fix the issue #18358
Usage of PLATFORM=$PLATFORM is expected to work, and will be needed where the azure pipeline builds are executing make commands with PLATFORM parameter.

Work item tracking
  • Microsoft ADO (number only):

How I did it

By unsetting the PLATFORM make flag in python-wheel generator targets in slave.mk

How to verify it

run the minimal build command make PLATFORM=marvell target/python-wheels/buster/sonic_config_engine-1.0-py2-none-any.whl --> To just build config-engine wheel package for buster as an example.
Verify the errors in below test cases of src/sonic-config-engine/tests/test_j2files.py is NOT seen.

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

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

Tested branch (Please provide the tested image version)

master

@krismarvell
Copy link
Copy Markdown
Contributor Author

@saiarcot895 Can you pls help with review and comment

@liushilongbuaa
Copy link
Copy Markdown
Contributor

I don't think it will work.
Official build didn't add 'PLATFORM=$PLATFORM' options when building target/sonic-vs.bin

@krismarvell
Copy link
Copy Markdown
Contributor Author

Official build didn't add 'PLATFORM=$PLATFORM' options when building target/sonic-vs.bin

Yes I agree with you on the official build comment. current builds doesnt have this option, but still this fix will be a foolproof for two reasons.

  1. this option may need to be used in make command for any specific use case (e.g. [Marvell] Build infrastructure enhancements  #18143 )
  2. intuitively it looked to me like even if PLATFORM=$PLATFORM is used in the make command after a make configure PLATFORM=$PLATFORM ideally it shud not fail the build.
    Pls let me know your comments.

Also, when you refer as this wont work, do you refer to any builds/platforms/options where you think it may not work? Pls help me understand if i have missed any specific UT/BUild types, i can give a try with this fix.

@saiarcot895 saiarcot895 self-requested a review April 8, 2024 17:46

# unset PLATFORM Flag for python wheel generation
PLATFORM=
export PLATFORM
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is it possible to have this be applied early in slave.mk, so that it applies for all builds, not just Python wheels?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It looks like anywhere else unsetting this outside specific targets, have the effect of globally changing the flag even for "make configure" as well. Hence have added only in the specific python wheel targets where this is hitting into the error.

@liushilongbuaa
Copy link
Copy Markdown
Contributor

Official build didn't add 'PLATFORM=$PLATFORM' options when building target/sonic-vs.bin

Yes I agree with you on the official build comment. current builds doesnt have this option, but still this fix will be a foolproof for two reasons.

  1. this option may need to be used in make command for any specific use case (e.g. [Marvell] Build infrastructure enhancements  #18143 )
  2. intuitively it looked to me like even if PLATFORM=$PLATFORM is used in the make command after a make configure PLATFORM=$PLATFORM ideally it shud not fail the build.
    Pls let me know your comments.

Also, when you refer as this wont work, do you refer to any builds/platforms/options where you think it may not work? Pls help me understand if i have missed any specific UT/BUild types, i can give a try with this fix.

I forgot it is for PR18143. sonic-py-common used env: PLATFORM. It makes sense to reset that variable when building image.

@TafkaMax
Copy link
Copy Markdown

TafkaMax commented Apr 9, 2024

I had the same issue when using .gitlab-ci.yml and using $PLATFORM as an env variable. More info here -> #18358

I hope this PR gets merged.

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.

5 participants