RobotWebTools / rclnodejs

Node.js version of ROS 2.0 client
https://docs.ros.org/en/humble/Concepts/Basic/About-Client-Libraries.html?highlight=rclnodejs#community-maintained
Apache License 2.0
311 stars 70 forks source link

Using galactic-geochelone binary packages for CI configuration on galactic-geochelone branch #830

Closed minggangw closed 2 years ago

minggangw commented 2 years ago

We should use the galactic-geochelone binary packages to verify on CIs instead of the latest binary, please see below:

https://github.com/RobotWebTools/rclnodejs/blob/cbfadd98ce877a16de03b560e24181f10a07f97d/appveyor.yml#L35-L36

https://github.com/RobotWebTools/rclnodejs/blob/cbfadd98ce877a16de03b560e24181f10a07f97d/Dockerfile#L35-L36

https://github.com/RobotWebTools/rclnodejs/blob/cbfadd98ce877a16de03b560e24181f10a07f97d/.circleci/config.yml#L50-L51

wayneparrott commented 2 years ago

@minggangw Is this what you are referring to by binary package?

# ros linux installation
sudo apt install ros-galactic-desktop
minggangw commented 2 years ago

As the daily build (or rolling) ROS2 binary may break the compiling on CIs due to the interface changes of rcl, we'd better use the stable release for its branch accordingly, e.g., galactic-geochelone uses the galactic binary package, and the develop branch will always use the daily release build to track the latest changes.

So, I suggest using the package downloaded from the Github release channel, because I noticed that our test needs test_interface_files package which doesn't exist when installing through sudo apt install ros-galactic-desktop, you can double-check whether it's still right.

The current change was introduced by https://github.com/RobotWebTools/rclnodejs/commit/12cd761277ba1affdcc32900a4aab3a3e81fad25, so we can change it back. image

wayneparrott commented 2 years ago

@minggangw apologies as I goofed up the ci ros distro access. Will fix asap. Reverting the galactic branch ci changes will still leave osx on the "rolling" distro as there are no official osx binaries beyond "foxy". As of the "galactic" release osx was downgraded to a Tier 3 platform.

minggangw commented 2 years ago

Reverting the galactic branch ci changes will still leave osx on the "rolling" distro as there are no official osx binaries beyond "foxy". As of the "galactic" release osx was downgraded to a Tier 3 platform.

Yeah, I think we have to drop the macOS platform, please #760

wayneparrott commented 2 years ago

Yeah, I think we have to drop the macOS platform, please #760

@minggangw Agree. Is the process for dropping osx as simple as removing the circleci workflow on the develop and galactic branches and updating docs?

minggangw commented 2 years ago

Yeah, removing the configuration file (config.yml) and updating the table (README) below is enough image

and we can remove the row for master branch status from the table also. Maybe we can remove the table completely and append the two badges (for Windows & Linux) after the other badges? image

wayneparrott commented 2 years ago

@minggangw 1) should we restrict installation of rclnodejs to macOs? e.g., add an os section to package.json

{
  "os": [
    "win32",
    "linux"
  ]
}

Alternatively, we could not restrict installation to macOs and instead provide a notice in the readme and npmjs-readme that macOs is not support; install at your own risk?

2) I tweaked the README to get this look in my dev env. The preview of the build-status table in vscode is omitting the borders. image Thoughts? If you like this or want a different layout please advise. Note: the dependencies service at david-dm.org has been offline for a while so I commented out the dependencies status label.

minggangw commented 2 years ago

should we restrict installation of rclnodejs to macOs?

I prefer not to restrict the installation on macOS, because I suppose users should be aware of the fact that there are no official ROS2 releases on macOS. Adding a notice in readme is fine.

I tweaked the README to get this look in my dev env. The preview of the build-status table in vscode is omitting the borders.

Looks good :) thanks for cleaning it up!

minggangw commented 2 years ago

I think we can close this as #832 was merged.