chore(deps): install dev tools and split if more repos #5528
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: Łukasz Dziedziak <lukidzi@gmail.com>
lukidzi
requested review from
a team,
lahabana and
michaelbeaumont
and removed request for
a team
Signed-off-by: Łukasz Dziedziak <lukidzi@gmail.com>
Signed-off-by: Łukasz Dziedziak <lukidzi@gmail.com>
michaelbeaumont
approved these changes
Dec 20, 2022
|
Why is this change required now and wasn't before? This sort of information should be in the commit message. |
Previously code was placed directly in Makefile and thanks to this we didn't have to pass TOOLS_DEPS_DIRS as a parameter to the script. When in another repository we are adding to env TOOLS_DEPS_DIRS another folder to scam it the value that enters the scripts is split by space which bash later recognizes as 2 separate parameters. That caused the other dir to be treated as GOOS. Signed-off-by: Łukasz Dziedziak <lukidzi@gmail.com>
jakubdyszkiewicz
approved these changes
Dec 20, 2022
michaelbeaumont
requested changes
Dec 20, 2022
There was a problem hiding this comment.
Feel free to dismiss this if not true but can't we just quote TOOLS_DEPS_DIRS in mk/dev.mk and use spaces?
$(TOOLS_DIR)/dev/install-dev-tools.sh $(CI_TOOLS_BIN_DIR) $(CI_TOOLS_DIR) "$(TOOLS_DEPS_DIRS)" $(TOOLS_DEPS_LOCK_FILE) $(GOOS) $(GOARCH)
Should be better than using , separator if the variable is known to be an array...
But then again it also doesn't matter either way 😀
Signed-off-by: Łukasz Dziedziak <lukidzi@gmail.com>
Looks like it works changed :) |
michaelbeaumont
approved these changes
Dec 20, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
syscall.Mkfifohave equivalent implementation on the other OS --UPGRADE.md? --> Changelog:entry here or add aci/label to run fewer/more tests?