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.

Sign up for GitHub

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

Jump to bottom

[build] 'make reset' target will continue recursive operations even if some fail #4675

Merged
merged 1 commit into from
Jun 1, 2020
Merged

[build] 'make reset' target will continue recursive operations even if some fail #4675

merged 1 commit into from
Jun 1, 2020

Conversation

jleveque
Copy link
Contributor

This change allows the recursive git clean and git reset commands to continue even if they encounter an error in one of the submodules. Previously, if an error was encountered, the operation would terminate with a message similar to the following:

Stopping at 'src/sonic-mgmt-framework'; script returned non-zero status.

@jleveque jleveque requested a review from lguohan May 30, 2020 21:03
@jleveque jleveque self-assigned this May 30, 2020
@jleveque
Copy link
Contributor Author

Retest vs please

@lguohan
Copy link
Collaborator

lguohan commented May 31, 2020

when would the command fail on the mgmt-framework submodule?

@lguohan
Copy link
Collaborator

lguohan commented May 31, 2020

retest vs please

@jleveque
Copy link
Contributor Author

@lguohan: It fails to delete files in the gopkgs subdirectory. This permissions issue needs further investigation. Some example messages:

...
warning: failed to remove gopkgs/pkg/mod/github.com/golang/protobuf@v1.3.4/proto/map_test.go: Permission denied
warning: failed to remove gopkgs/pkg/mod/github.com/golang/protobuf@v1.3.4/proto/properties.go: Permission denied
warning: failed to remove gopkgs/pkg/mod/github.com/golang/protobuf@v1.3.4/ptypes/duration_test.go: Permission denied
warning: failed to remove gopkgs/pkg/mod/github.com/golang/protobuf@v1.3.4/ptypes/timestamp/timestamp.pb.go: Permission denied
...

@jleveque
Copy link
Contributor Author

Retest vsimage please

@lguohan
Copy link
Collaborator

lguohan commented May 31, 2020

retest vsimage please

@lguohan
Copy link
Collaborator

lguohan commented May 31, 2020

why in this case, we should skip these errors and let it continue? shouldn't we ask broadcom to fix the reset issue on their repo?

@jleveque
Copy link
Contributor Author

@lguohan: Yes, Broadcom should fix the root cause, but this patch allows the command to continue and reset the remaining submodules. The error messages still get printed to stdout, so it will be noticed, it's not suppressed. This patch just prevents the need to manually clean up multiple submodules if any one fails to clean up.

@lguohan
Copy link
Collaborator

lguohan commented May 31, 2020

retest vsimage please

lguohan
lguohan reviewed Jun 1, 2020
View reviewed changes
Makefile.work Show resolved Hide resolved
@lguohan
Copy link
Collaborator

lguohan commented Jun 1, 2020

when you merge, can you make sure you edited the commit message to include what you have in the pr.

@jleveque jleveque merged commit 336cf2a into sonic-net:master Jun 1, 2020
@jleveque jleveque deleted the make_reset_ignore_errors branch June 1, 2020 20:28
abdosi pushed a commit that referenced this pull request Jun 3, 2020
@jleveque @abdosi
[build] 'make reset' target will continue recursive operations if any…
f9bb26f 
… fail (#4675)

This change allows the recursive `git clean` and `git reset` commands to continue even if they encounter an error in one of the submodules. Previously, if an error was encountered, the operation would terminate with a message similar to the following:

Stopping at 'src/sonic-mgmt-framework'; script returned non-zero status.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lguohan lguohan lguohan approved these changes

Assignees

@jleveque jleveque

Labels
Enhancement ➕ Included in 201911 Branch Request for 201911 Branch :shipit:
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jleveque @lguohan @abdosi