Fix Makefile for Windows builds #566

RivenSkaye · 2023-07-27T10:12:54Z

RivenSkaye commented

2023-07-27 10:12:54 +00:00

Fixes #564 (/gitea/tea/issues/564) by applying minor changes to the Makefile.

Defines a BINEXT variable which holds a file name extension (.exe for Windows, empty otherwise)
Replaces uses of $(EXECUTABLE) with tea$(EXECUTABLE)
appends $(BINEXT) to the custom vet tool flag

Besides that I made two minor changes

Add [.exe] to the ignore rule for gitea-vet, so it also ignores the binary when built on Windows hosts
Removed $(DIST)/binaries from release-dirs as this is not being called anywhere else

I've tested almost all make tasks to work on Windows. The only one I haven't run is the docker build.

WIP tag is because I still have to test it on Linux, will do so in a bit. I also asked for some discussion on the original issue, which I'd like to do before blindly asking to merge in changes

Fixes #564 (/gitea/tea/issues/564) by applying minor changes to the Makefile. - Defines a `BINEXT` variable which holds a file name extension (.exe for Windows, empty otherwise) - Replaces uses of `$(EXECUTABLE)` with `tea$(EXECUTABLE)` - appends `$(BINEXT)` to the custom vet tool flag Besides that I made two minor changes - Add `[.exe]` to the ignore rule for gitea-vet, so it also ignores the binary when built on Windows hosts - Removed `$(DIST)/binaries` from release-dirs as this is not being called anywhere else I've tested almost all `make` tasks to work on Windows. The only one I haven't run is the docker build. WIP tag is because I still have to test it on Linux, will do so in a bit. I also asked for some discussion on the original issue, which I'd like to do before blindly asking to merge in changes

RivenSkaye added 3 commits 2023-07-27 10:12:58 +00:00

Fix #564 EXECUTABLE => BINEXT (Makefile) 8b41b831a2

remove unused $(DIST)/binaries dir e95b14b7bb

Also ignore gitea-vet on Windows

check-and-test / check-and-test (pull_request) Successful in 54s

Details

c75256b87c

RivenSkaye commented

2023-07-27 14:27:36 +00:00

All tests run fine on Linux as well, so it's pretty much good to go if the solution is well-received.
There are, however, two things to note.

I forgot to sign off on the commits. I can revert and redo it, but I'd rather not since I'm not that experienced with reverting or amending history
getting the linter suite and whatnot to work required adding GOPATH/bin to my system path (clean system install). I didn't notice on Windows because I already had it in my PATH there, but under the umbrella of environment compatibility I'd like to also edit the revive and misspell lines to prefix them with $(shell $(GO) env GOPATH)/bin/.

All tests run fine on Linux as well, so it's pretty much good to go if the solution is well-received. There are, however, two things to note. 1. I forgot to sign off on the commits. I can revert and redo it, but I'd rather not since I'm not _that_ experienced with reverting or amending history 2. getting the linter suite and whatnot to work required adding `GOPATH/bin` to my system path (clean system install). I didn't notice on Windows because I already had it in my PATH there, but under the umbrella of environment compatibility I'd like to also edit the revive and misspell lines to prefix them with `$(shell $(GO) env GOPATH)/bin/`.

jolheiser commented

2023-07-27 15:11:49 +00:00

I wonder if it would be better to do something similar for the vet tool rather than deal with BINEXT.

# OS specific vars.
ifeq ($(OS), Windows_NT)
	EXECUTABLE := tea.exe
+   VET_TOOL := gitea-vet.exe
else
	EXECUTABLE := tea
+   VET_TOOL := gitea-vet.exe
	ifneq ($(shell uname -s), OpenBSD)
		override BUILDMODE := -buildmode=pie
	endif
endif

I'm on the fence, so either seems fine to me.

I forgot to sign off on the commits. I can revert and redo it, but I'd rather not since I'm not that experienced with reverting or amending history

For signing off, I'm not too concerned since you've signed the commits and have indicated here as an ack.

getting the linter suite and whatnot to work required adding GOPATH/bin to my system path (clean system install). I didn't notice on Windows because I already had it in my PATH there, but under the umbrella of environment compatibility I'd like to also edit the revive and misspell lines to prefix them with $(shell $(GO) env GOPATH)/bin/.

For this I think a go run directive would be more useful to directly run them, but it doesn't need to be included in this PR. 🙂

I wonder if it would be better to do something similar for the vet tool rather than deal with `BINEXT`. ```diff # OS specific vars. ifeq ($(OS), Windows_NT) EXECUTABLE := tea.exe + VET_TOOL := gitea-vet.exe else EXECUTABLE := tea + VET_TOOL := gitea-vet.exe ifneq ($(shell uname -s), OpenBSD) override BUILDMODE := -buildmode=pie endif endif ``` I'm on the fence, so either seems fine to me. > I forgot to sign off on the commits. I can revert and redo it, but I'd rather not since I'm not that experienced with reverting or amending history For signing off, I'm not too concerned since you've signed the commits and have indicated here as an ack. > getting the linter suite and whatnot to work required adding GOPATH/bin to my system path (clean system install). I didn't notice on Windows because I already had it in my PATH there, but under the umbrella of environment compatibility I'd like to also edit the revive and misspell lines to prefix them with $(shell $(GO) env GOPATH)/bin/. For this I think a `go run` directive would be more useful to directly run them, but it doesn't need to be included in this PR. 🙂

RivenSkaye commented

2023-07-27 15:37:17 +00:00

I wonder if it would be better to do something similar for the vet tool

I thought about this as well before opening the issue (I made sure it actually worked with correct filenames first), but if more files get added to the make tasks, it'd also require more variables which is why I went with this. But I'll gladly leave the final decision to maintainers and opt out of blame 😉

but it doesn't need to be included in this PR.

Gotcha, I'll do a quick search for other issues and open one if there aren't any once I'm home. I'll also make the requested changes then

> I wonder if it would be better to do something similar for the vet tool I thought about this as well before opening the issue (I made sure it actually worked with correct filenames first), but if more files get added to the `make` tasks, it'd also require more variables which is why I went with this. But I'll gladly leave the final decision to maintainers and opt out of blame 😉 > but it doesn't need to be included in this PR. Gotcha, I'll do a quick search for other issues and open one if there aren't any once I'm home. I'll also make the requested changes then

❤️ 1

RivenSkaye referenced this pull request

2023-07-28 06:13:33 +00:00

Bug: lint and release tools not found under specific circumstances #567

RivenSkaye added 1 commit 2023-07-28 06:18:55 +00:00

Proposed changes from @jolheiser

Signed-off-by: Martin Veldwijk <riven@tae.moe>

check-and-test / check-and-test (pull_request) Failing after 26s

Details

860e651307

RivenSkaye added 1 commit 2023-07-28 06:23:12 +00:00

Fix typo

Signed-off-by: Martin Veldwijk <riven@tae.moe>

check-and-test / check-and-test (pull_request) Successful in 55s

Details

e709f6b547

RivenSkaye changed title from ~~WIP: Fix Makefile for Windows builds~~ to Fix Makefile for Windows builds

2023-07-28 06:27:47 +00:00

RivenSkaye commented

2023-07-28 06:29:04 +00:00

Didn't make it last night, something came up. CI run passed, so if there's anything else missing I'd gladly fix before approval

lunny approved these changes 2023-07-28 09:17:21 +00:00

jolheiser approved these changes 2023-07-28 14:03:01 +00:00

jolheiser merged commit 77837e909e into main

2023-07-28 14:03:32 +00:00

jolheiser referenced this issue from a commit

2023-07-28 14:03:40 +00:00

Fix Makefile for Windows builds (#566)

6543 added the

kind/bug

kind/build

labels 2023-08-21 22:02:03 +00:00

6543 added this to the v0.10.0 milestone 2023-08-21 22:02:05 +00:00

Sign in to join this conversation.

No reviewers

No Milestone

No Assignees

3 Participants

Notifications

Due Date

The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: gitea/tea#566