-
Notifications
You must be signed in to change notification settings - Fork 958
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
FreeBSD Node support #678
FreeBSD Node support #678
Conversation
Signed-off-by: hackacad <admin@hackacad.net>
✅ DCO Check Passed 745343c |
Why wouldn't we be using the packaged version? |
The packaged version is Linux ELF and not compatible with FreeBSD |
Same question as for Java in OpenSearch. Would it be better if we made a FreeBSD package with the appropriate node version? |
@hackacad thanks for the change! Can you respond to dB's question above? Also, can we automate the testing for this? |
The FreeBSD port will always use the node shipped via FreeBSD (ports). It would of course be possible to integrate one, but as you already mentioned this would require a full CI pipeline for testing on FreeBSD. |
Hello @hackacad, apologies on the delay. We will try to get some prioritization on this. |
Seems pretty innocent to me. Bump @kavilla. |
|
||
if [ $OS = "freebsd" ]; then | ||
NODE="/usr/local/bin/node" | ||
else | ||
NODE="${DIR}/node/bin/node" | ||
fi | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another possibility (maybe simpler): if a bundled node
is present, use it, otherwise expect one to bie found in $PATH:
if [ $OS = "freebsd" ]; then | |
NODE="/usr/local/bin/node" | |
else | |
NODE="${DIR}/node/bin/node" | |
fi | |
if [ -x "${DIR}/node/bin/node" ]; then | |
NODE="${DIR}/node/bin/node" | |
else | |
NODE="node" | |
fi | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about a
elif [ ! -x "$(which node)" ]; then
looks a little bit more solid then using node
without a path. Not sure if has any side effects.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's probably better and allow less modifications of the rest of the script.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hackacad were you able to get insight getting node without the path.
For me, i think the original code for me I can pretty explicitly read that we are doing this conditional for FreeBSD node which makes me feel safer.
(basically the originally change is ok for me)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using a more general condition looks better, but it should still look for a path. Using node
without a path would cover most environments, but looking for it (like using which) should be much more solid.
if [ $OS = "freebsd" ]; then | ||
NODE="/usr/local/bin/node" | ||
else | ||
NODE="${DIR}/node/bin/node" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NODE=which node
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just want to clarify here - are you planning on making additional changes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just added the improved and more liberal node selection (OS independent)
Signed-off-by: Sven R <admin@hackacad.net>
✅ DCO Check Passed c80da20 |
Signed-off-by: Sven R <admin@hackacad.net>
✅ DCO Check Passed eddeb2d |
elif [ -x "$(which node)" ]; then | ||
NODE="$(which node)" | ||
fi | ||
|
||
test -x "$NODE" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Useless test. The same test happen 2 lines bellow in a condition. This line can be removed.
Signed-off-by: Sven R <admin@hackacad.net>
✅ DCO Check Passed 23865dd |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great! I appreciate the simplification.
Now that we have a more general solution, I think there are some final steps to close things out:
- Change the title and description of the PR
- Squash and amend the commits to reflect the changes that were made
- Add details to the DEVELOPER_GUIDE to explain what is supported.
@hackacad really appreciate the contribute. I am trying to launch a FreeBSD node in AWS but can't make it due to yarn & node version issue. Can you help us to test whether it will cause any backward compatibility issue to break the build? For example:
then if you pull the newest commit, you should be able to run
and
and etc. Could you help us to test a bit more? I think other than this, we are good to go. Thanks. |
Of course, we can support you in setting up an OpenSearch FreeBSD instance. |
A quick message to add the link to the discord server where some members of the FreeBSD OpenSearch group are hanging out: OpenSearch Community (unofficial) |
Hello @hackacad thanks for the support. I have launched several freeBSD instances (freeBSD13-stable-amd and freeBSD11-stable-amd). To run
The supported yarn versions are
The yarn-1.22.11 will remove node 10 and install node 16
So in freeBSD, it seems dashboards can't be build directly. I would suggest use PR #795 and #840 as the references to add a build for freeBSD. For example, when run |
@ananzh can you use nvm to get node 10 for build purposes? (e.g. https://github.com/carlosgruiz-dev/carlosgruiz-dev/wiki/FreeBSD:-Install-nvm) (should only be needed until we go v16) |
Hey @stockholmux I tried this yesterday and it doesn't work. Here is the result:
If search gcc7 by |
EOL node needs EOL gcc… legacy dependency hell 🙃 We (FreeBSD) avoid distributing software which has reached End Of Life. We currently support node 14 (LTS) and 16 (current), so I had to adjust this: nvim ./package.json diff --git a/package.json b/package.json
index b50a51e755..235d29a7d5 100644
--- a/package.json
+++ b/package.json
@@ -468,7 +468,7 @@
"zlib": "^1.0.5"
},
"engines": {
- "node": "10.24.1",
+ "node": "14.17.6",
"yarn": "^1.21.1"
}
} Then I could successfully run the commands you wrote:
Also note that the FreeBSD port has this patch (not needed for building): Not sure what you wanted to do @ananzh, hope this help! Running just Hey @smortex thanks for the confirm. I opened an issue here to remind us testing and verifying the build of dashboards once it is bumped to node 16. I will approve it. Thanks a lot for the contribution. |
@hackacad thanks again for the great work. would you like to merge the PR to main? I could help to backport it to 1.2 and 1.x. |
probably could be backported just to the |
Backport PR: opensearch-project#678 Signed-off-by: hackacad <admin@hackacad.net>
Backport PR: #678 Signed-off-by: hackacad <admin@hackacad.net> Co-authored-by: hackacad <admin@hackacad.net>
Signed-off-by: hackacad admin@hackacad.net
Description
Adding upstream support for FreeBSD
Issues Resolved
Using default path for node on FreeBSD