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

Making cross compile independent of gox vender package #2047

Merged
merged 8 commits into from
Aug 28, 2019

Conversation

amitkrout
Copy link
Contributor

What is the purpose of this change? What does it change?

Was the change discussed in an issue?

#1998 (comment)

How to test changes?

make cross

@amitkrout amitkrout changed the title Making cross compile independent of gox vender package [WIP] Making cross compile independent of gox vender package Aug 26, 2019
@openshift-ci-robot openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. Required by Prow. label Aug 26, 2019
@mohammedzee1000
Copy link
Contributor

mohammedzee1000 commented Aug 26, 2019

You can make this cleaner by doing it in a shell script and calling it here I think
or having separate targets for each os like how it's done for oc and call all of them for cross

@amitkrout amitkrout changed the title [WIP] Making cross compile independent of gox vender package Making cross compile independent of gox vender package Aug 26, 2019
@openshift-ci-robot openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. Required by Prow. label Aug 26, 2019
@amitkrout
Copy link
Contributor Author

.PHONY: cross
cross:
    @for platform in linux darwin windows ; do \
        if [ $$platform == "windows" ]; then \
        IS_EXE := .exe; \
        fi \
        go build -o dist/bin/$$platform-amd64/odo$(IS_EXE) $(BUILD_FLAGS) ./cmd/odo/ \
    done

I prefer this over the change i made. But unfortunately the syntax fix took longer time without success. If anybody fix the error please comment cc_ @mohammedzee1000 @kadel

Makefile Outdated
@@ -79,10 +79,27 @@ goget-tools:
test-coverage:
./scripts/generate-coverage.sh

# compile for linux platform
Copy link
Contributor

@mohammedzee1000 mohammedzee1000 Aug 26, 2019

Choose a reason for hiding this comment

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

Maybe set GOOS before doing it :

env GOOS=linux go build -blah

Copy link
Contributor Author

@amitkrout amitkrout Aug 26, 2019

Choose a reason for hiding this comment

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

@mohammedzee1000 Something like ?

.PHONY: darwin-amd64
darwin-amd64:
	GOARCH=amd64 GOOS=darwin go build $(BUILD_FLAGS) -o dist/bin/darwin-amd64/odo ./cmd/odo/

Copy link
Contributor

Choose a reason for hiding this comment

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

yes

Makefile Outdated
gox -osarch="darwin/amd64 linux/amd64 windows/amd64" -output="dist/bin/{{.OS}}-{{.Arch}}/odo" $(BUILD_FLAGS) ./cmd/odo/
@for platform in linux darwin windows ; do \
if [ $$platform == "windows" ]; then \
echo "Cross compiling $$platform-amd64 and placing binary at dist/bin/$$platform-amd64/"; \
Copy link
Contributor

Choose a reason for hiding this comment

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

You can put the echo before if as its common

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could not get it. Can you please explain it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it, thanks
Will update it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@amitkrout
Copy link
Contributor Author

.PHONY: cross
cross:
    @for platform in linux darwin windows ; do \
        if [ $$platform == "windows" ]; then \
        IS_EXE := .exe; \
        fi \
        go build -o dist/bin/$$platform-amd64/odo$(IS_EXE) $(BUILD_FLAGS) ./cmd/odo/ \
    done

I prefer this over the change i made. But unfortunately the syntax fix took longer time without success. If anybody fix the error please comment cc_ @mohammedzee1000 @kadel

Finally able to fix the syntax error.

@kadel
Copy link
Member

kadel commented Aug 27, 2019

Please remove gox from makefile and from other places if it is mentioned somewhere

Copy link
Contributor

@mohammedzee1000 mohammedzee1000 left a comment

Choose a reason for hiding this comment

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

Looking good
/approve

@openshift-ci-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mohammedzee1000

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. Required by Prow. label Aug 27, 2019
@mohammedzee1000
Copy link
Contributor

It works, although as stated by @kadel remove remaining references

@amitkrout
Copy link
Contributor Author

amitkrout commented Aug 27, 2019

Please remove gox from makefile and from other places if it is mentioned somewhere

Done

$ git grep -n "gox"
Binary file tests/examples/binary/java/openjdk/sb.jar matches
Binary file vendor/github.com/openshift/origin/docs/proposals/wireframes/Project-logs.png matches
vendor/golang.org/x/text/language/display/tables.go:16418:      "ongoxinès simplificatxinès tradicional"

@amitkrout
Copy link
Contributor Author

[odo]  ✗  Failed To Update Config To Component Deployed
[odo]  ✗  Applying configuration [55s]

/test integration

@amitkrout
Copy link
Contributor Author

/refresh

@mohammedzee1000
Copy link
Contributor

• Failure [156.587 seconds]
odoJavaE2e
/go/src/github.com/openshift/odo/tests/integration/java_test.go:13
  odo component creation
  /go/src/github.com/openshift/odo/tests/integration/java_test.go:41
    Should be able to deploy a spring boot uberjar file using openjdk [It]
    /go/src/github.com/openshift/odo/tests/integration/java_test.go:94
     Failed after 90 retries. Content in http://uberjaropenjdk-app-phppkfrfmn.apps.ci-op-dr740dig-f09f4.origin-ci-int-aws.dev.rhcloud.com doesn't include 'HTTP Booster'.
     /go/src/github.com/openshift/odo/tests/helper/helper_http.go:41
------------------------------
 Summarizing 1 Failure:
 [Fail] odoJavaE2e odo component creation [It] Should be able to deploy a spring boot uberjar file using openjdk 
/go/src/github.com/openshift/odo/tests/helper/helper_http.go:41
 Ran 4 of 97 Specs in 315.104 seconds
FAIL! -- 3 Passed | 1 Failed | 0 Pending | 93 Skipped

Something up with springboot

@mohammedzee1000
Copy link
Contributor

/retest

@amitkrout
Copy link
Contributor Author

/test integration

@kadel
Copy link
Member

kadel commented Aug 28, 2019

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. Required by Prow. label Aug 28, 2019
@kadel
Copy link
Member

kadel commented Aug 28, 2019

/retest

@openshift-merge-robot openshift-merge-robot merged commit 2fbb154 into redhat-developer:master Aug 28, 2019
cdrage added a commit to cdrage/odo that referenced this pull request Sep 3, 2019
- Lists all URLs even if they are undeployed or not
(redhat-developer#2034)

- Added json output for `odo project delete -o json`
(redhat-developer#2037)

- Fixed service integration test that was previously failing
(redhat-developer#2022)

- `odo push` will now only push changed files
(redhat-developer#2030)
- We now use relative paths within the file indexer
(redhat-developer#2003)
- You can now create list and edit services using --app and --project
(redhat-developer#2001)
- Deleted files will now propagate to the OpenShift container
(redhat-developer#1999)
- Added `odo service create --context` functionality
(redhat-developer#1997)
- Making cross-compile independant of gox vendor package
(redhat-developer#2047)
- `odo-supervisord-image` has been renamed to `odo-init-image`
(redhat-developer#2027)
- Releases now use .tar.gz (redhat-developer#2009)
- If there is an error creating a service, it will fail quicker
(redhat-developer#2008)

- We now have a Google Group!
(redhat-developer#2007)
- Added documentation on how to manage environment
variables(redhat-developer#2026)
- Badges added to the README
(redhat-developer#2060)
- Updated documentation on uninstallation
(redhat-developer#2053)
- Added documentation for default parameters
(redhat-developer#2038)
- Minor update to help output
(redhat-developer#2006)
- Updated documentation regarding bootstrapper image
(redhat-developer#1991)
@cdrage cdrage mentioned this pull request Sep 3, 2019
openshift-merge-robot pushed a commit that referenced this pull request Sep 3, 2019
- Lists all URLs even if they are undeployed or not
(#2034)

- Added json output for `odo project delete -o json`
(#2037)

- Fixed service integration test that was previously failing
(#2022)

- `odo push` will now only push changed files
(#2030)
- We now use relative paths within the file indexer
(#2003)
- You can now create list and edit services using --app and --project
(#2001)
- Deleted files will now propagate to the OpenShift container
(#1999)
- Added `odo service create --context` functionality
(#1997)
- Making cross-compile independant of gox vendor package
(#2047)
- `odo-supervisord-image` has been renamed to `odo-init-image`
(#2027)
- Releases now use .tar.gz (#2009)
- If there is an error creating a service, it will fail quicker
(#2008)

- We now have a Google Group!
(#2007)
- Added documentation on how to manage environment
variables(#2026)
- Badges added to the README
(#2060)
- Updated documentation on uninstallation
(#2053)
- Added documentation for default parameters
(#2038)
- Minor update to help output
(#2006)
- Updated documentation regarding bootstrapper image
(#1991)
@rm3l rm3l added the estimated-size/S (5-10) Rough sizing for Epics. Less then one sprint of work for one person label Jun 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. Required by Prow. estimated-size/S (5-10) Rough sizing for Epics. Less then one sprint of work for one person lgtm Indicates that a PR is ready to be merged. Required by Prow.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants