kie-issues#1566: Remove pkg dependency from image-env-to-json on kie-tools, as it's deprecated
#2901
+1,002
−403
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
thiagoelg
reviewed
Feb 11, 2025
packages/image-env-to-json/Makefile
Outdated
| build-all: build-darwin-amd64 build-darwin-arm64 build-linux-amd64 build-linux-arm64 build-win-amd64 build-win-arm64 | ||
|
|
||
| build-darwin: | ||
| CGO_ENABLED=0 GOOS=darwin GOARCH=$(GOARCH) go build -ldflags $(LDFLAGS) -o $(BIN_PATH)/$(BIN_BUILD_DARWIN) $(MAIN_PATH) |
There was a problem hiding this comment.
I know it's not ideal, but in other packages, we use cross-env here to avoid OS-specific errors when cross-building.
thiagoelg
reviewed
Feb 11, 2025
Comment on lines
17
to
31
| "build": "run-script-os", | ||
| "build:darwin": "pnpm setup:env make build-darwin", | ||
| "build:darwin:amd": "pnpm setup:env make build-darwin-amd64", | ||
| "build:darwin:arm": "pnpm setup:env make build-darwin-arm64", | ||
| "build:dev": "rimraf -rf dist && pnpm build", | ||
| "build:linux": "pnpm setup:env make build-linux", | ||
| "build:linux:amd": "pnpm setup:env make build-linux-amd64", | ||
| "build:linux:arm": "pnpm setup:env make build-linux-arm64", | ||
| "build:prod": "rimraf -rf dist && run-script-os && pnpm test", | ||
| "build:prod:darwin": "rimraf dist && pnpm setup:env make build-all", | ||
| "build:prod:linux": "rimraf dist && pnpm setup:env make build-all", | ||
| "build:prod:win32": "rimraf dist && pnpm setup:env:win32 make build-all", | ||
| "build:win32": "pnpm setup:env:win32 make build-win", | ||
| "build:win32:amd": "pnpm setup:env:win32 make build-win-amd64", | ||
| "build:win32:arm": "pnpm setup:env:win32 make build-win-arm64", |
There was a problem hiding this comment.
I think we should build all binaries every time (on build:dev and build:prod). It should be quick, and we can avoid issues where, for example, we test the binary in a macOS ARM environment but then run it inside a Linux x64 container.
It would also simplify this list of scripts in the package.json.
thiagoelg
reviewed
Feb 11, 2025
| "install": "go mod tidy", | ||
| "powershell": "@powershell -NoProfile -ExecutionPolicy Unrestricted -Command", | ||
| "setup:env": "cross-env PLUGIN_VERSION=$(build-env imageEnvToJson.version)", | ||
| "setup:env:win32": "pnpm powershell \"cross-env PLUGIN_VERSION=$(build-env imageEnvToJson.version)\"", |
There was a problem hiding this comment.
Is powershell needed even with cross-env?
There was a problem hiding this comment.
Yes, we need to set the environment variables before the make.
b349758 to
d4cfcdb
Compare
d4cfcdb to
4255190
Compare
|
Please add tests for when env vars contain JSON strings too. Thank you! |
tiagobento
approved these changes
Feb 21, 2025
thiagoelg
approved these changes
Feb 21, 2025
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.
Closes: apache/incubator-kie-issues#1566
Closes: #2578
Description
Fully rewrite using Golang to simplify generate native binaries.
Obs: Some usages weren't updated to use the new Darwin and Windows binaries, since they are placed inside a Linux container.