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

Remove version printing #71

Merged
merged 4 commits into from
Aug 20, 2024
Merged

Conversation

Ryanf55
Copy link
Contributor

@Ryanf55 Ryanf55 commented Nov 17, 2023

Logs are now clean (at least from MicroXRCEDDSGen)

$ colcon build --packages-up-to ardupilot_sitl
Starting >>> micro_ros_agent
--- stderr: micro_ros_agent                              
Cloning into 'xrceagent'...
Switched to a new branch 'ros2'
HEAD is now at c25243c Enable Domain Override on Reference and XML Participant (#351)
CMake Warning (dev) at /usr/share/cmake-3.22/Modules/FindPackageHandleStandardArgs.cmake:438 (message):
  The package name passed to `find_package_handle_standard_args` (tinyxml2)
  does not match the name of the calling package (TinyXML2).  This can lead
  to problems in calling code that expects `find_package` result variables
  (e.g., `_FOUND`) to follow a certain pattern.
Call Stack (most recent call first):
  cmake/modules/FindTinyXML2.cmake:40 (find_package_handle_standard_args)
  /opt/ros/humble/share/fastrtps/cmake/fastrtps-config.cmake:51 (find_package)
  CMakeLists.txt:153 (find_package)
This warning is for project developers.  Use -Wno-dev to suppress it.

---
Finished <<< micro_ros_agent [4.49s]
Starting >>> ardupilot_sitl
Finished <<< ardupilot_sitl [2.99s]                  

Summary: 2 packages finished [8.21s]
  1 package had stderr output: micro_ros_agent

@pablogs9
Copy link
Member

Please remove also

java -version > NUL 2>&1
and
java -version &>/dev/null

@Ryanf55 Ryanf55 force-pushed the remove-version-print branch from f2ea127 to 65101d4 Compare November 29, 2023 04:42
@Ryanf55
Copy link
Contributor Author

Ryanf55 commented Nov 29, 2023

Please remove also

java -version > NUL 2>&1

and

java -version &>/dev/null

Looking at these, it seems the point of printing the version is actually to get an exit code for later logic based on if the script exists.

Since the intent is to check whether the program exists, the following is a recommended solution:
https://stackoverflow.com/questions/592620/how-can-i-check-if-a-program-exists-from-a-bash-script

Let me know if you like the new approach. I think a similar one can be taken on the .bat file.

@pablogs9
Copy link
Member

I like this approach of checking if java command is available.

@Ryanf55
Copy link
Contributor Author

Ryanf55 commented Nov 29, 2023

Ok. I'll try the same out for windows. I will need you to test since I don't have a windows dev platform.

@Ryanf55
Copy link
Contributor Author

Ryanf55 commented May 25, 2024

Can you please review this? We're still getting terminal span from builds, and the other devs don't like it outputting on stderr.

@pablogs9
Copy link
Member

Did you test this on windows? I do not have a windows dev platform.

@Ryanf55
Copy link
Contributor Author

Ryanf55 commented May 27, 2024

Nope. If we don't have any windows platforms, what about reverting the windows change and we can just merge Linux?

@pablogs9
Copy link
Member

Sure

@Ryanf55 Ryanf55 force-pushed the remove-version-print branch from ecb6f1e to 95a44c5 Compare August 1, 2024 15:03
@Ryanf55
Copy link
Contributor Author

Ryanf55 commented Aug 1, 2024

HI Pablo, I dropped the windows commit. Please take another review. We are continuing to have users report problems during installation due to the stderr of this package.

@pablogs9
Copy link
Member

pablogs9 commented Aug 2, 2024

You have added a bunch of commits from my feature/v3. Could you keep this with the last commit only?

* It's on stderr and noisy

Signed-off-by: Ryan Friedman <[email protected]>
@Ryanf55 Ryanf55 force-pushed the remove-version-print branch from 95a44c5 to 5a7cf02 Compare August 6, 2024 14:58
@Ryanf55
Copy link
Contributor Author

Ryanf55 commented Aug 6, 2024

You have added a bunch of commits from my feature/v3. Could you keep this with the last commit only?

Done. Sorry, this PR was branched off of Ardupilot master, which includes some stuff from feature/v3 we're using. It's clean now.

@pablogs9 pablogs9 changed the base branch from master to develop August 20, 2024 05:48
@pablogs9 pablogs9 merged commit cfffa50 into eProsima:develop Aug 20, 2024
3 checks passed
@Ryanf55 Ryanf55 deleted the remove-version-print branch August 20, 2024 14:31
Ryanf55 added a commit to ArduPilot/Micro-XRCE-DDS-Gen that referenced this pull request Aug 24, 2024
* It's on stderr and noisy

Signed-off-by: Ryan Friedman <[email protected]>
Co-authored-by: Antonio Cuadros <[email protected]>
Co-authored-by: Pablo Garrido <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants