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

Bump version of smallstep/nosql to newest to fix build error on illumos #3602

Closed
wants to merge 1 commit into from

Conversation

Toasterson
Copy link

No description provided.

@CLAassistant
Copy link

CLAassistant commented Jul 25, 2020

CLA assistant check
All committers have signed the CLA.

@mholt
Copy link
Member

mholt commented Jul 25, 2020

Unfortunately I don't think we can accept this change, because it causes a regression: #3526 (edit: er, more specifically, this issue: #3525 -- it looks like we're still waiting for Dgraph to tag a new release, then for nosql to tag a new release that uses it.)

I might be wrong, but that was my understanding. @mohammed90 would probably be able to confirm either way.

@mholt mholt requested a review from mohammed90 July 25, 2020 16:17
@mholt mholt added the do not merge ⛔ Not ready yet! label Jul 25, 2020
@Toasterson
Copy link
Author

The json in the Issue mentioned does not get rejected for me, but also does not produce output when calling.

I changed the port to 80 to get a http server running which works but that server returns empty content.

{
   "apps":{
      "http":{
         "servers":{
            "srv0":{
               "listen":[
                  ":80"
               ],
               "routes":[
                  {
                     "match":[
                        {
                           "expression":"{http.request.uri.query} != ''"
                        }
                     ],
                     "handle":[
                        {
                           "handler":"static_response",
                           "body":"Hello"
                        }
                     ]
                  }
               ]
            }
         }
      }
   }
}
root@build:~# ./caddyv2 run -config caddy.json 
2020/07/25 16:18:24 WARNING: proto: file "pb.proto" is already registered
A future release will panic on registration conflicts. See:
https://developers.google.com/protocol-buffers/docs/reference/go/faq#namespace-conflict

2020/07/25 16:18:24.256 INFO     using provided configuration    {"config_file": "caddy.json", "config_adapter": ""}
2020/07/25 16:18:24.257 INFO     admin   admin endpoint started  {"address": "tcp/localhost:2019", "enforce_origin": false, "origins": ["[::1]:2019", "127.0.0.1:2019", "localhost:2019"]}
2020/07/25 16:18:24 [INFO][cache:0xc00035c360] Started certificate maintenance routine
2020/07/25 16:18:24.266 INFO     http    server is listening only on the HTTP port, so no automatic HTTPS will be applied to this server {"server_name": "srv0", "http_port": 80}
2020/07/25 16:18:24.267 INFO     tls     cleaned up storage units
2020/07/25 16:18:24.267 INFO     autosaved config        {"file": "/root/.config/caddy/autosave.json"}
2020/07/25 16:18:24.267 INFO     serving initial configuration
root@build:~# curl -i localhost:80
HTTP/1.1 200 OK
Server: Caddy
Date: Sat, 25 Jul 2020 16:21:06 GMT
Content-Length: 0

@mohammed90
Copy link
Member

mohammed90 commented Jul 25, 2020

Matt is right. The PR #3526 resolves 2 issues, one of them is the warning printed due to conflict pb registration in BadgerDB. The warning is, in fact, printed in your log:

root@build:~# ./caddyv2 run -config caddy.json 
2020/07/25 16:18:24 WARNING: proto: file "pb.proto" is already registered
A future release will panic on registration conflicts. See:
https://developers.google.com/protocol-buffers/docs/reference/go/faq#namespace-conflict

@Toasterson
Copy link
Author

So this only a warning right now? I can live with that locally until a new release is tagged or I can bump everything for a local build for only me. I guess you want a tagged release for that on your side. Or is the response broken and caddy should respond with content but doesn't?

@Toasterson
Copy link
Author

Toasterson commented Jul 25, 2020

Ah I need to do

root@build:~# curl -i localhost?hi=hello; echo
HTTP/1.1 200 OK
Server: Caddy
Date: Sat, 25 Jul 2020 16:38:41 GMT
Content-Length: 5

Hello

Shall I close this and you look into it when a tagged release is here or how shall we proceed?

@mohammed90
Copy link
Member

Ah I need to do

root@build:~# curl -i localhost?hi=hello; echo
HTTP/1.1 200 OK
Server: Caddy
Date: Sat, 25 Jul 2020 16:38:41 GMT
Content-Length: 5

Hello

Shall I close this and you look into it when a tagged release is here or how shall we proceed?

Yeah, we'll have to close this for now. Sorry. We'll re-visit the version bump once smallstep/nosql depends on a release of badgerdb that includes the commit dgraph-io/badger@8097259. But I'm intrigued, can you share more about the illumos build error, since it presumably affects Caddy?

@mohammed90 mohammed90 closed this Jul 25, 2020
@Toasterson
Copy link
Author

Yeah sure. The main issue is that badger@1.5.3 uses golangs internal syscall library and that does not support many operatingsystems notably openbsd, netbsd, and illumos. Badger uses a constant for MADVISE and that only came into the go for these Unixes after the internal syscall library got frozen. Right now the internal syscall library only really works for linux and every software that wants to support something else than linux and uses the syscall library directly must use x/sys/unix instead. I've personally changed quite a few direct dependencies and pushed people to use x/sys/unix instead of syscall so that today this is mostly happening due the dependency chain. I could not upgrade badger to 1.6.1 as there where api changes but nosql bump worked. So until that is resolved caddy cannot be built on quite a few unix versions I suppose. I know it won't build for illumos right now. Are you monitoring cross-builds?

@Toasterson
Copy link
Author

Also here the exact error for documentation

# github.com/dgraph-io/badger/y
../../../pkg/mod/github.com/dgraph-io/badger@v1.5.3/y/mmap_unix.go:57:30: undefined: syscall.SYS_MADVISE

@dcow
Copy link

dcow commented Aug 16, 2020

FWIW that protobuf warning isn't regarding any functional issue in most cases. The new protobuf lib just decided to start printing a warning instead of silently ignoring the issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
do not merge ⛔ Not ready yet!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants