Skip to content
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

[makefile] define a do-nothing target for config.user #7483

Merged
merged 3 commits into from
Apr 30, 2021

Conversation

yxieca
Copy link
Contributor

@yxieca yxieca commented Apr 29, 2021

Why I did it

After PR #7344, 'make init' and/or 'make reset' will also build sonic slave dockers.

'-include rules/config.user' is supposed to be fine when the file is missing. However, when the file is missing, it generates a delayed error which later causes make init and make reset trying to build the sonic slave dockers.

How I did it

Define a do-nothing target for config.user to catch config.user build therefore preventing other builds to be triggered unexpectedly.

How to verify it

did make init and it is now only doing submodule init.

@qiluo-msft
Copy link
Collaborator

It seems missing by design 7e2fa7d. What issue are you facing?

@yxieca
Copy link
Contributor Author

yxieca commented Apr 29, 2021

It seems missing by design 7e2fa7d. What issue are you facing?

as in why I did it :-)
After PR #7344, 'make init' and/or 'make reset' will also build sonic slave dockers.

@yxieca
Copy link
Contributor Author

yxieca commented Apr 29, 2021

-include is supposed to be fine when file is missing. checking more.

@yxieca yxieca force-pushed the make branch 3 times, most recently from bb8441a to 88142ab Compare April 29, 2021 23:43
-include rules/config.user is supposed to be fine when the file is missing.
However, it triggers an interesting issue that when the file is missing,
make init and make reset will also try to build the sonic slave dockers.

Use the less exciting way to include it only when it exists works around
the unexpected docker build issue.

Signed-off-by: Ying Xie <[email protected]>
@yxieca
Copy link
Contributor Author

yxieca commented Apr 29, 2021

@qiluo-msft you are right the design was that the file can be missing.

@bluecmd can you take a look at this change. This is less exciting then your original change but work-around the issue that sonic slave docker got built with "make init" and "make reset". If you have better way of preventing it, please let me know or propose with another PR.

@yxieca yxieca changed the title [makefile] add missing rules/config.user [makefile] use a less exciting way to include config.user Apr 29, 2021
@bluecmd
Copy link
Contributor

bluecmd commented Apr 30, 2021

Hi!

While I can't say I noticed this behavior when I made the change, I found this passage in the Make docs that might be the reason why the include seems to cause other things to build:

If an included makefile cannot be found in any of these directories, a warning message is generated, but it is not an immediately fatal error; processing of the makefile containing the include continues. Once it has finished reading makefiles, make will try to remake
https://www.gnu.org/software/make/manual/html_node/Include.html

Reading that, it seems that this could possibly also be fixed by explicitly defining rules/config.user as an empty target that can't be made. Maybe like this (100% untested!):

rules/config.user:

include rules/config
-include rules/config.user

Personally, I think the wildcard approach here is pretty smart and nice looking - but I also know we have -include rules/config.user in at least one other place, so defining this target once might fix it for everyone.

What do you say about giving this other way a try and see if it works and then we can see which approach seems the right way forward?

@yxieca yxieca changed the title [makefile] use a less exciting way to include config.user [makefile] define a do-nothing target for config.user Apr 30, 2021
@yxieca
Copy link
Contributor Author

yxieca commented Apr 30, 2021

@bluecmd Thanks for digging up the information. The do-nothing target works with a slight tweak. The target needs at least 1 dummy operations to catch the rebuild. But it works! Thanks!

@yxieca yxieca merged commit 5da0046 into sonic-net:master Apr 30, 2021
@yxieca yxieca deleted the make branch April 30, 2021 20:04
daall pushed a commit that referenced this pull request May 10, 2021
Why I did it
After PR #7344, 'make init' and/or 'make reset' will also build sonic slave dockers.

'-include rules/config.user' is supposed to be fine when the file is missing. However, when the file is missing, it generates a delayed error which later causes make init and make reset trying to build the sonic slave dockers.

How I did it
Define a do-nothing target for config.user to catch config.user build therefore preventing other builds to be triggered unexpectedly.

How to verify it
did make init and it is now only doing submodule init.
raphaelt-nvidia pushed a commit to raphaelt-nvidia/sonic-buildimage that referenced this pull request May 23, 2021
Why I did it
After PR sonic-net#7344, 'make init' and/or 'make reset' will also build sonic slave dockers.

'-include rules/config.user' is supposed to be fine when the file is missing. However, when the file is missing, it generates a delayed error which later causes make init and make reset trying to build the sonic slave dockers.

How I did it
Define a do-nothing target for config.user to catch config.user build therefore preventing other builds to be triggered unexpectedly.

How to verify it
did make init and it is now only doing submodule init.
carl-nokia pushed a commit to carl-nokia/sonic-buildimage that referenced this pull request Aug 7, 2021
Why I did it
After PR sonic-net#7344, 'make init' and/or 'make reset' will also build sonic slave dockers.

'-include rules/config.user' is supposed to be fine when the file is missing. However, when the file is missing, it generates a delayed error which later causes make init and make reset trying to build the sonic slave dockers.

How I did it
Define a do-nothing target for config.user to catch config.user build therefore preventing other builds to be triggered unexpectedly.

How to verify it
did make init and it is now only doing submodule init.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants