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

FreeBSD Node support #678

Merged
merged 4 commits into from
Oct 19, 2021
Merged

Conversation

hackacad
Copy link
Contributor

Signed-off-by: hackacad [email protected]

Description

Adding upstream support for FreeBSD

Issues Resolved

Using default path for node on FreeBSD

Signed-off-by: hackacad <[email protected]>
@opensearch-ci-bot
Copy link
Collaborator

✅   DCO Check Passed 745343c

@dblock
Copy link
Member

dblock commented Jul 27, 2021

Why wouldn't we be using the packaged version?

@hackacad
Copy link
Contributor Author

The packaged version is Linux ELF and not compatible with FreeBSD

@dblock
Copy link
Member

dblock commented Jul 27, 2021

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?

@tmarkley
Copy link
Contributor

tmarkley commented Aug 3, 2021

@hackacad thanks for the change! Can you respond to dB's question above?

Also, can we automate the testing for this?

@hackacad
Copy link
Contributor Author

hackacad commented Aug 3, 2021

The FreeBSD port will always use the node shipped via FreeBSD (ports).
There're a couple of reasons (like one port for all release versions, not like on Linux Distros).

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.

@kavilla
Copy link
Member

kavilla commented Aug 31, 2021

Hello @hackacad, apologies on the delay. We will try to get some prioritization on this.

@dblock
Copy link
Member

dblock commented Sep 7, 2021

Seems pretty innocent to me. Bump @kavilla.

Comment on lines 29 to 35

if [ $OS = "freebsd" ]; then
NODE="/usr/local/bin/node"
else
NODE="${DIR}/node/bin/node"
fi

Copy link

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:

Suggested change
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

Copy link
Contributor Author

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.

Copy link

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.

Copy link
Member

@kavilla kavilla Sep 17, 2021

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)

Copy link
Contributor Author

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.

@kavilla
Copy link
Member

kavilla commented Sep 13, 2021

Screen Shot 2021-09-13 at 1 16 21 AM

if [ $OS = "freebsd" ]; then
NODE="/usr/local/bin/node"
else
NODE="${DIR}/node/bin/node"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

NODE=which node

Copy link
Contributor

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?

Copy link
Contributor Author

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)

@kavilla kavilla self-requested a review September 20, 2021 21:33
kavilla
kavilla previously approved these changes Sep 20, 2021
@opensearch-ci-bot
Copy link
Collaborator

✅   DCO Check Passed c80da20

Signed-off-by: Sven R <[email protected]>
@opensearch-ci-bot
Copy link
Collaborator

✅   DCO Check Passed eddeb2d

elif [ -x "$(which node)" ]; then
NODE="$(which node)"
fi

test -x "$NODE"
Copy link

@smortex smortex Sep 22, 2021

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.

@opensearch-ci-bot
Copy link
Collaborator

✅   DCO Check Passed 23865dd

Copy link

@smortex smortex left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@tmarkley tmarkley left a 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:

  1. Change the title and description of the PR
  2. Squash and amend the commits to reflect the changes that were made
  3. Add details to the DEVELOPER_GUIDE to explain what is supported.

@ananzh
Copy link
Member

ananzh commented Sep 29, 2021

@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:

yarn build

then if you pull the newest commit, you should be able to run

yarn build-platform --linux

and

yarn build-platform --darwin

and etc.

Could you help us to test a bit more? I think other than this, we are good to go. Thanks.

@ananzh ananzh self-requested a review September 29, 2021 16:35
@kavilla
Copy link
Member

kavilla commented Oct 12, 2021

@hackacad any updates to what @ananzh asked?

Thanks!

@hackacad
Copy link
Contributor Author

@hackacad any updates to what @ananzh asked?

Thanks!

Of course, we can support you in setting up an OpenSearch FreeBSD instance.
You can send an email to the FreeBSD team ([email protected]);
@stockholmux has access to our Discord channel as well

@smortex
Copy link

smortex commented Oct 14, 2021

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)

@ananzh
Copy link
Member

ananzh commented Oct 15, 2021

@hackacad any updates to what @ananzh asked?
Thanks!

Of course, we can support you in setting up an OpenSearch FreeBSD instance. You can send an email to the FreeBSD team ([email protected]); @stockholmux has access to our Discord channel as well

Hello @hackacad thanks for the support. I have launched several freeBSD instances (freeBSD13-stable-amd and freeBSD11-stable-amd). To run yarn build or run dashboards, we need node version set to 10.24.1 and need to use yarn. In freeBSD13 and freeBSD11, the supported node versions are

pkg search node
kadnode-2.3.0                  P2P name resolution daemon
leafnode-1.11.12               NNTP package for offline news caching and reading
munin-node-2.0.67              Node-specific part of Munin
node-16.10.0                   V8 JavaScript for client and server
node-thrift-0.14.0             Node.js bindings for the Apache Thrift RPC system
node10-10.24.1_1               V8 JavaScript for client and server
node14-14.17.6                 V8 JavaScript for client and server
node_exporter-1.2.2            Prometheus exporter for machine metrics
npm-node14-6.14.8              Node package manager
p5-Tree-DAG_Node-1.32          Super class for representing nodes in a tree
p5-Tree-Node-0.08_2            Memory-efficient tree nodes in Perl
p5-WebService-Linode-0.29      Perl Interface to the Linode.com API
p5-XML-Node-0.11_1             Perl5 module to extend and simplify XML::Parser
p5-XML-NodeFilter-0.01_1       XML::NodeFilter is an object that know how to "filter out" nodes
py38-certbot-dns-linode-1.18.0 Linode DNS Authenticator plugin for Certbot
py38-node-semver-0.8.0         Python version of node-semver
py38-nodeenv-1.5.0             Node.js virtual environment builder
yarn-node14-1.22.11            Package manager for node, alternative to npm

The supported yarn versions are

pkg search yarn
yarn-1.22.11                   Package manager for node, alternative to npm
yarn-node14-1.22.11            Package manager for node, alternative to npm

The yarn-1.22.11 will remove node 10 and install node 16

Installed packages to be REMOVED:
node10: 10.24.1_1
 
New packages to be INSTALLED:
node: 16.10.0
yarn: 1.22.11

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 yarn build-platform --freeBSD a freeBSD artifact can be built in any system env. Then we could upload this freeBSD artifact from local env to a freeBSD instance then test it.

@stockholmux
Copy link
Member

stockholmux commented Oct 15, 2021

@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)

@ananzh
Copy link
Member

ananzh commented Oct 15, 2021

@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:

root@freebsd:/home/ec2-user # pkg install gcc7 gmake
Updating FreeBSD repository catalogue...
FreeBSD repository is up to date.
All repositories are up to date.
pkg: No packages available to install matching 'gcc7' have been found in the repositories

If search gcc7 by pkg search gcc7 there is no found. I think the best way is to build a freeBSD artifact in ubuntu with any related PRs then cp to freeBSD instance.

@smortex
Copy link

smortex commented Oct 16, 2021

If search gcc7 by pkg search gcc7 there is no found. I think the best way is to build a freeBSD artifact in ubuntu with any related PRs then cp to freeBSD instance.

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:

sudo pkg install yarn-node14 python3 gmake
yarn osd bootstrap
yarn build-platform --linux
yarn build-platform --darwin

Also note that the FreeBSD port has this patch (not needed for building):
https://cgit.freebsd.org/ports/tree/textproc/opensearch-dashboards/files/patch-src_setup__node__env_node__version__validator.js

Not sure what you wanted to do @ananzh, hope this help! Running just yarn build I hit sass/sass#1873 (which I believe is not a FreeBSD problem).

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.

@ananzh
Copy link
Member

ananzh commented Oct 18, 2021

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

@kavilla kavilla merged commit 915a13c into opensearch-project:main Oct 19, 2021
@kavilla
Copy link
Member

kavilla commented Oct 19, 2021

@ananzh,

probably could be backported just to the 1.x branch.

ananzh pushed a commit to ananzh/OpenSearch-Dashboards that referenced this pull request Oct 20, 2021
@ananzh ananzh mentioned this pull request Oct 20, 2021
5 tasks
ananzh added a commit that referenced this pull request Oct 21, 2021
Backport PR:
#678

Signed-off-by: hackacad <[email protected]>

Co-authored-by: hackacad <[email protected]>
@ananzh ananzh added the v1.2.0 label Nov 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants