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

allmark: fix build #352579

Merged
merged 1 commit into from
Nov 14, 2024
Merged

allmark: fix build #352579

merged 1 commit into from
Nov 14, 2024

Conversation

Bot-wxt1221
Copy link
Member

@Bot-wxt1221 Bot-wxt1221 commented Oct 31, 2024

Fix build

We can partly backport it actually.

I wonder what it the best solution.

ZHF: #352882

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 24.11 Release Notes (or backporting 23.11 and 24.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

Copy link
Member

@Aleksanaa Aleksanaa left a comment

Choose a reason for hiding this comment

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

The old repo has not been deleted, and the new repo has only one commit and no star. The author of the old repo did not recommend people to switch to the new repo.

If your intention is fixing build, you can port essential fixes here.

@Bot-wxt1221 Bot-wxt1221 changed the title allmark: switch to github.com/hmmftg/allmark allmark: fix build Oct 31, 2024
@Bot-wxt1221
Copy link
Member Author

@Aleksanaa Done.

pkgs/by-name/al/allmark/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/al/allmark/0001-fix-build.patch Outdated Show resolved Hide resolved
@Bot-wxt1221
Copy link
Member Author

@Aleksanaa Done.

@Bot-wxt1221 Bot-wxt1221 requested a review from Aleksanaa November 2, 2024 10:27
@Bot-wxt1221 Bot-wxt1221 added the 0.kind: ZHF Fixes Fixes during the Zero Hydra Failures (ZHF) campaign label Nov 2, 2024
@itepastra itepastra mentioned this pull request Nov 2, 2024
13 tasks
Copy link
Contributor

@zi3m5f zi3m5f left a comment

Choose a reason for hiding this comment

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

Here the actual build errors:
common/config/config.go:22:2: cannot find module providing package github.com/abbot/go-http-auth: import lookup disabled by -mod=vendor
common/config/config.go:23:2: cannot find module providing package github.com/mitchellh/go-homedir: import lookup disabled by -mod=vendor
dataaccess/filesystem/watcher.go:11:2: cannot find module providing package github.com/andreaskoch/go-fswatch: import lookup disabled by -mod=vendor
services/imageconversion/resizer.go:9:2: cannot find module providing package github.com/nfnt/resize: import lookup disabled by -mod=vendor
services/converter/markdowntohtml/postprocessor/emoji.go:8:2: cannot find module providing package github.com/kyokomi/emoji: import lookup disabled by -mod=vendor
services/converter/markdowntohtml/markdowntohtml.go:14:2: cannot find module providing package github.com/russross/blackfriday: import lookup disabled by -mod=vendor
web/orchestrator/search/fulltextindex.go:11:2: cannot find module providing package github.com/andreaskoch/fulltext: import lookup disabled by -mod=vendor
web/orchestrator/search/fulltextindex.go:12:2: cannot find module providing package github.com/spf13/afero: import lookup disabled by -mod=vendor
web/handlers/update/connection.go:10:2: cannot find module providing package golang.org/x/net/websocket: import lookup disabled by -mod=vendor
web/handlers/compression.go:4:2: cannot find module providing package github.com/gorilla/handlers: import lookup disabled by -mod=vendor
web/server/server.go:23:2: cannot find module providing package github.com/gorilla/mux: import lookup disabled by -mod=vendor
web/server/server.go:24:2: cannot find module providing package github.com/skratchdot/open-golang/open: import lookup disabled by -mod=vendor

It would help the review if you provide more than just "fix build" ;)

As suggested in #353186 switching to go 1.22 with buildGo122Module would be a simpler fix in the short term.

I saw @urandom2 created andreaskoch/allmark#37 and his PR commits could be used instead of postPatch and your own 0001-Add-go.mod-go.sum-remove-vendor.patch (how did you create it?).

I tested using the PR 37 commits locally and it works too. Patch:
diff --git a/pkgs/by-name/al/allmark/package.nix b/pkgs/by-name/al/allmark/package.nix
index 1fcdfce5f6bd..72b9a54276df 100644
--- a/pkgs/by-name/al/allmark/package.nix
+++ b/pkgs/by-name/al/allmark/package.nix
@@ -2,6 +2,7 @@
   lib,
   buildGoModule,
   fetchFromGitHub,
+  fetchpatch,
 }:
 
 buildGoModule rec {
@@ -15,11 +16,22 @@ buildGoModule rec {
     hash = "sha256-JfNn/e+cSq1pkeXs7A2dMsyhwOnh7x2bwm6dv6NOjLU=";
   };
 
-  postPatch = ''
-    go mod init github.com/andreaskoch/allmark
-  '';
+  patches = [
+    # add go module
+    (fetchpatch {
+      url = "https://github.com/andreaskoch/allmark/pull/37/commits/168fa0a194a9cdc15ba22cae154402bf8939168c.patch";
+      hash = "sha256-PyJgkRFNg91RnLEezmaS9u+4D8AD1g2g3OJs+AhKms0=";
+    })
+    # replace vendor directory with go.mod and go.sum
+    (fetchpatch {
+      url = "https://github.com/andreaskoch/allmark/pull/37/commits/5e26a550abaed18d00b88677d6def15c5d84ce05.patch";
+      includes = [ "go.mod" "go.sum" ];
+      hash = "sha256-E2fTWkJzgDAtlqVJooIkX06lc+pw8GiGmCq+5Vd64UA=";
+    })
+  ];
 
-  vendorHash = null;
+  deleteVendor = true;
+  vendorHash = "sha256-32MDPl+8lfUqyhG0rjZhpGMJzgIvb2E0xOTQ6wDKJto=";
 
   postInstall = ''
     mv $out/bin/{cli,allmark}

But not sure what's best 🤷‍♂️

pkgs/by-name/al/allmark/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/al/allmark/package.nix Outdated Show resolved Hide resolved
@Bot-wxt1221
Copy link
Member Author

Bot-wxt1221 commented Nov 3, 2024

@zi3m5f Done. I'd love to just keep it like this. We need to replace go-cdb to cdb as well. I create patch by partly backporting from https://github.com/hmmftg/allmark.

Anyway, patch from https://github.com/andreaskoch/allmark/pull/37/files should be too big. It remove vendor in this patch.

@Bot-wxt1221 Bot-wxt1221 requested a review from zi3m5f November 3, 2024 07:28
Copy link
Contributor

@zi3m5f zi3m5f left a comment

Choose a reason for hiding this comment

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

Anyway, patch from https://github.com/andreaskoch/allmark/pull/37/files should be too big. It remove vendor in this patch.

includes = [ ... ]; results in only part of the patch being put in the nix store.

Generally, I ask myself why would someone use such outdated software, is there no up to date and maintained fork, and what about security/ CVEs?

Regardless, not my decision ;)

pkgs/by-name/al/allmark/package.nix Show resolved Hide resolved
@Bot-wxt1221
Copy link
Member Author

@zi3m5f Maybe apply patch from PR is the best solution.

@Bot-wxt1221 Bot-wxt1221 requested a review from zi3m5f November 3, 2024 12:01
Copy link
Contributor

@zi3m5f zi3m5f left a comment

Choose a reason for hiding this comment

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

Maybe apply patch from PR is the best solution.

Ideally @urandom2 would have some insight on this.

Otherwise @luftmensch-luftmensch as maintainer and @Aleksanaa can probably better decide 🤷‍♂️

nixpkgs-review result

Generated using nixpkgs-review.

Command: nixpkgs-review pr 352579


x86_64-linux

✅ 1 package built:
  • allmark

@Bot-wxt1221 Bot-wxt1221 requested a review from zi3m5f November 8, 2024 02:09
@zi3m5f
Copy link
Contributor

zi3m5f commented Nov 8, 2024

Why do you request a review from me again?

I already wrote after your last request:

Ideally @urandom2 would have some insight on this.
Otherwise @luftmensch-luftmensch as maintainer and @Aleksanaa can probably better decide 🤷‍♂️

Please stop doing that without at least answering/reacting to my comment, thanks.

@wegank wegank marked this pull request as draft November 10, 2024 00:47
@Bot-wxt1221 Bot-wxt1221 marked this pull request as ready for review November 10, 2024 07:39
@Bot-wxt1221
Copy link
Member Author

I think the best solution is apply patch from PR and I do it.

@GaetanLepage
Copy link
Contributor

Maybe you should use fetchpatch2 instead of fetchpatch. I couldn't find clear guidelines on this question though.

@Bot-wxt1221
Copy link
Member Author

@GaetanLepage Are there any diffrences between them?

@GaetanLepage
Copy link
Contributor

@GaetanLepage Are there any diffrences between them?

I asked on matrix and apparently, using fetchpatch is fine for this case.

@GaetanLepage
Copy link
Contributor

nixpkgs-review result

Generated using nixpkgs-review.

Command: nixpkgs-review pr 352579


x86_64-linux

✅ 1 package built:
  • allmark

aarch64-linux

✅ 1 package built:
  • allmark

x86_64-darwin

❌ 1 package failed to build:
  • allmark

aarch64-darwin

❌ 1 package failed to build:
  • allmark

@Bot-wxt1221
Copy link
Member Author

@GaetanLepage Oh darwin again. Could you tell me why darwin fails because it succeed in ofborg.

@GaetanLepage
Copy link
Contributor

@GaetanLepage Oh darwin again. Could you tell me why darwin fails because it succeed in ofborg.

Network issues apparently:

Running phase: checkPhase
panic: No free port found between 34000 and 34000

goroutine 1 [running]:
github.com/andreaskoch/allmark/common/ports.GetFreePort({0x102509079, 0x4}, {{0x14000102120, 0x10, 0x10}, 0x84cf, {0x0, 0x0}})
        /private/tmp/nix-build-allmark-0.10.0.drv-1/source/common/ports/ports.go:36 +0x108
github.com/andreaskoch/allmark/common/config.(*TCPBinding).AssignFreePort(0x1400012c280)
        /private/tmp/nix-build-allmark-0.10.0.drv-1/source/common/config/config.go:255 +0xa0
github.com/andreaskoch/allmark/common/config.init.0()
        /private/tmp/nix-build-allmark-0.10.0.drv-1/source/common/config/config.go:89 +0x12c
FAIL    github.com/andreaskoch/allmark/common/config    0.184s
FAIL

Consider adding __darwinAllowLocalNetworking = true;

@Bot-wxt1221
Copy link
Member Author

@GaetanLepage Could you test again?

@Bot-wxt1221
Copy link
Member Author

@ofborg build allmark

@GaetanLepage
Copy link
Contributor

nixpkgs-review result

Generated using nixpkgs-review.

Command: nixpkgs-review pr 352579


x86_64-linux

✅ 1 package built:
  • allmark

aarch64-linux

✅ 1 package built:
  • allmark

x86_64-darwin

✅ 1 package built:
  • allmark

aarch64-darwin

✅ 1 package built:
  • allmark

Copy link
Contributor

@GaetanLepage GaetanLepage left a comment

Choose a reason for hiding this comment

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

LGTM, thanks !

@Bot-wxt1221 Bot-wxt1221 force-pushed the allmark branch 2 times, most recently from 3a6c85c to 6418107 Compare November 14, 2024 07:47
@Bot-wxt1221
Copy link
Member Author

@Aleksanaa What do you mean? Add pr's url to here?

I mean

Patches already merged upstream or published elsewhere should be retrieved using fetchpatch.

Otherwise, you can add a .patch file to the nixpkgs repository. In the interest of keeping our maintenance burden and the size of nixpkgs to a minimum, only do this for patches that are unique to nixpkgs or that have been proposed upstream but are not merged yet, cannot be easily fetched or have a high chance to disappear in the future due to unstable or unreliable URLs. The latter avoids link rot when the upstream abandons, squashes or rebases their change, in which case the commit may get garbage-collected.

We'd better keep patch in nixpkgs like what I did before, so now I fallback to the old.

@Bot-wxt1221
Copy link
Member Author

@ofborg build allmark

@Aleksanaa Aleksanaa merged commit 0f24588 into NixOS:master Nov 14, 2024
13 of 15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants