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

Adding support for windows-arm64 #457

Merged
merged 4 commits into from
Aug 10, 2022
Merged

Adding support for windows-arm64 #457

merged 4 commits into from
Aug 10, 2022

Conversation

koushdey
Copy link
Contributor

This PR adds changes to support ORAS CLI on windows 64 Architecture.

Signed-off-by: Koushik Dey 57134947+koushdey@users.noreply.github.com

@koushdey
Copy link
Contributor Author

This PR is a follow up PR of this #455

@koushdey
Copy link
Contributor Author

cc: @qweeah @shizhMSFT @phoenix24

shizhMSFT
shizhMSFT approved these changes Jul 23, 2022
View reviewed changes
Copy link
Contributor

@shizhMSFT shizhMSFT left a comment

Choose a reason for hiding this comment

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

LGTM

shizhMSFT
shizhMSFT requested changes Jul 23, 2022
View reviewed changes
Copy link
Contributor

@shizhMSFT shizhMSFT left a comment

Choose a reason for hiding this comment

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

@koushdey The CI test failed. Could you fix that before merging?

shizhMSFT
shizhMSFT requested changes Jul 26, 2022
View reviewed changes
Makefile Outdated
build-windows:
build-windows: build-windows-amd64 build-windows-arm64

.PHONY: build-windows-amd64:
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems there is an extra colon : after amd64.

@codecov-commenter
Copy link

codecov-commenter commented Jul 29, 2022 •
edited
Loading

Codecov Report

Merging #457 (4cd116c) into main (d776a22) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main     #457   +/-   ##
=======================================
  Coverage   55.69%   55.69%           
=======================================
  Files           6        6           
  Lines         237      237           
=======================================
  Hits          132      132           
  Misses         90       90           
  Partials       15       15

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

shizhMSFT
shizhMSFT approved these changes Jul 29, 2022
View reviewed changes
Copy link
Contributor

@shizhMSFT shizhMSFT left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM

shizhMSFT
shizhMSFT requested changes Jul 29, 2022
View reviewed changes
Copy link
Contributor

@shizhMSFT shizhMSFT left a comment

Choose a reason for hiding this comment

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

@koushdey The makefile still does not look right. Could you revise according to the given suggestion?

Makefile Outdated
Comment on lines 67 to 77
build-windows: build-windows-amd64 build-windows-arm64

.PHONY: build-windows-amd64
GOARCH=amd64 CGO_ENABLED=0 GOOS=windows go build -v --ldflags="$(LDFLAGS)" \
-o bin/windows/amd64/$(CLI_EXE).exe $(CLI_PKG)

.PHONY: build-windows-arm64
build-windows:
GOARCH=arm64 CGO_ENABLED=0 GOOS=windows go build -v --ldflags="$(LDFLAGS)" \
-o bin/windows/arm64/$(CLI_EXE).exe $(CLI_PKG)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
build-windows: build-windows-amd64 build-windows-arm64
.PHONY: build-windows-amd64
GOARCH=amd64 CGO_ENABLED=0 GOOS=windows go build -v --ldflags="$(LDFLAGS)" \
-o bin/windows/amd64/$(CLI_EXE).exe $(CLI_PKG)
.PHONY: build-windows-arm64
build-windows:
GOARCH=arm64 CGO_ENABLED=0 GOOS=windows go build -v --ldflags="$(LDFLAGS)" \
-o bin/windows/arm64/$(CLI_EXE).exe $(CLI_PKG)
build-windows: build-windows-amd64 build-windows-arm64
.PHONY: build-windows-amd64
build-windows-amd64:
GOARCH=amd64 CGO_ENABLED=0 GOOS=windows go build -v --ldflags="$(LDFLAGS)" \
-o bin/windows/amd64/$(CLI_EXE).exe $(CLI_PKG)
.PHONY: build-windows-arm64
build-windows-arm64:
GOARCH=arm64 CGO_ENABLED=0 GOOS=windows go build -v --ldflags="$(LDFLAGS)" \
-o bin/windows/arm64/$(CLI_EXE).exe $(CLI_PKG)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added the suggestion. Please have a look

qweeah
qweeah approved these changes Aug 2, 2022
View reviewed changes
Copy link
Contributor

@qweeah qweeah left a comment

Choose a reason for hiding this comment

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

LGTM

@koushdey
Copy link
Contributor Author

koushdey commented Aug 2, 2022

@shizhMSFT @qweeah Can you merge the PR if all looks good?

shizhMSFT
shizhMSFT reviewed Aug 2, 2022
View reviewed changes
Makefile
build-windows:
build-windows: build-windows-amd64 build-windows-arm64
.PHONY: build-windows-amd64
build-windows-amd64:
Copy link
Contributor

Choose a reason for hiding this comment

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

A empty line is required between makefile items for consistency and readability.

For example:

a:
    Build a

b:
    Build b

c:
    Build c

Copy link
Contributor

Choose a reason for hiding this comment

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

I just checked if modified target can be made, it's better to add empty line between items.

@shizhMSFT
Copy link
Contributor

@koushdey Could you format the makefile as requested?

@qweeah
Copy link
Contributor

qweeah commented Aug 3, 2022

@shizhMSFT @qweeah Can you merge the PR if all looks good?

Sorry I can't. I am not a maintainer :P

koushdey and others added 4 commits August 10, 2022 16:41
@koushdey @shizhMSFT
Adding support for windows-arm64
0cd2a8a 
Signed-off-by: Koushik Dey <57134947+koushdey@users.noreply.github.com>
@koushdey @shizhMSFT
Fixing failures in makefile
131736b 
Signed-off-by: Koushik Dey <57134947+koushdey@users.noreply.github.com>
@koushdey @shizhMSFT
Addressed review comments
5f4fd34 
Signed-off-by: Koushik Dey <57134947+koushdey@users.noreply.github.com>
@shizhMSFT
format Makefile with newlines
4cd116c 
Signed-off-by: Shiwei Zhang <shizh@microsoft.com>
shizhMSFT
shizhMSFT approved these changes Aug 10, 2022
View reviewed changes
Copy link
Contributor

@shizhMSFT shizhMSFT left a comment

Choose a reason for hiding this comment

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

LGTM. Formatted with newlines to unblock merging.

@shizhMSFT shizhMSFT merged commit 7c6fbdb into oras-project:main Aug 10, 2022
TerryHowe pushed a commit to TerryHowe/oras that referenced this pull request Feb 2, 2023
@koushdey
Adding support for windows-arm64 (oras-project#457)
d5c80de 
Signed-off-by: Koushik Dey <57134947+koushdey@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@qweeah qweeah qweeah approved these changes

@shizhMSFT shizhMSFT shizhMSFT approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@koushdey @codecov-commenter @shizhMSFT @qweeah