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

[ADDED] Ability to build on NetBSD #3526

Merged
merged 1 commit into from Oct 28, 2022
Merged

[ADDED] Ability to build on NetBSD #3526

merged 1 commit into from Oct 28, 2022

Conversation

MatthiasPetermann
Copy link
Contributor

Signed-off-by: Matthias Petermann mp@petermann-it.de

  • Link to issue, e.g. Resolves #NNN
  • Documentation added (if applicable)
  • [-] Tests added
  • Branch rebased on top of current main (git pull --rebase origin main)
  • [-] Changes squashed to a single commit (described here)
  • [?] Build is green in Travis CI
  • You have certified that the contribution is your original work and that you license the work to the project under the Apache 2 license

Resolves #3525

Changes proposed in this pull request:

  • Add pse support on NetBSD (based on OpenBSD work)
  • Add (dummy) support for disk availability on NetBSD (similiar to Windows work) - I will work on real support on NetBSD later

/cc @nats-io/core

kozlovic
kozlovic requested changes Oct 6, 2022
View reviewed changes
Copy link
Member

@kozlovic kozlovic left a comment

Choose a reason for hiding this comment

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

Since it seems that you said you would look at a proper implementation, how urgent do we need this PR to be merged? Is that a blocker for you?

We are planning on a v2.9.3 release, and I may not want to include that there. I am yet to create a "dev" branch with new feature/breaking change PRs should be posted against.

@MatthiasPetermann
Copy link
Contributor Author

Hello, thank you very much for your reply. Please also excuse my late response. I have looked again at the facts of the statfs syscall. In fact it is unfortunately the case that this is already not supported in the Golang standard library (for NetBSD). Since this would be the right place for an appropriate addition, the best choice for the nats-server at the moment is to continue with the dummy implementation, as is the case in the Windows variant. Would this be possible?

@kozlovic
Copy link
Member

the best choice for the nats-server at the moment is to continue with the dummy implementation, as is the case in the Windows variant. Would this be possible?

So you mean as you have it in this PR, where diskAvailable returns JetStreamMaxStoreDefault, correct?

I mean, we do this for Windows (and wasm stub), so I guess this would be ok if there is no other alternative. @derekcollison what do you think?

@derekcollison
Copy link
Member

ok by me.

@kozlovic
Copy link
Member

@MatthiasPetermann Ok, could you modify your PR to be made against the branch called "dev" (instead of main)? We are trying to keep changes/additions for the next release separate from the main branch. Let me know if there is any issue with that.

@MatthiasPetermann MatthiasPetermann changed the base branch from main to dev October 19, 2022 10:47
@MatthiasPetermann
Copy link
Contributor Author

Hello, thank you for your assessment. I have now changed the pull request to the dev branch. Please let me know if there is anything else to do. Kind regards, Matthias

kozlovic
kozlovic requested changes Oct 19, 2022
View reviewed changes
Copy link
Member

@kozlovic kozlovic left a comment

Choose a reason for hiding this comment

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

@MatthiasPetermann Sorry for not catching this early. Just a nit about copyright and then we will be able to merge. Also, if you don't mind, could you squash and have the commit's title be something like:

[ADDED] Ability to build on NetBSD

This will help us later when writing the release notes to easily see the commits that need to be included there. Thanks!

server/disk_avail.go Show resolved Hide resolved
server/disk_avail_netbsd.go Outdated
@@ -0,0 +1,22 @@
// Copyright 2021 The NATS Authors
Copy link
Member

Choose a reason for hiding this comment

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

New file, so please change this year to 2022.

server/pse/pse_netbsd.go Outdated
@@ -0,0 +1,36 @@
// Copyright 2015-2018 The NATS Authors
Copy link
Member

Choose a reason for hiding this comment

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

Same, new file, so simply use 2022 for the year (instead of this range).

@MatthiasPetermann
Copy link
Contributor Author

Hello, thanks for the tips. I made the small changes and in the meantime also learned how to combine several previous commits ;-) I would be happy if you could look over the updated pull request again when you get a chance. If more changes are needed, please don't hesitate to write me. Kind regards, Matthias

@kozlovic kozlovic changed the title Enable nats-server to build on NetBSD [ADDED] Ability to build on NetBSD Oct 28, 2022
kozlovic
kozlovic approved these changes Oct 28, 2022
View reviewed changes
Copy link
Member

@kozlovic kozlovic 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 for the contribution!

@kozlovic kozlovic merged commit 6d03d75 into nats-io:dev Oct 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kozlovic kozlovic kozlovic 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.

Enable nats-server to be built on NetBSD
3 participants
@MatthiasPetermann @kozlovic @derekcollison