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

Suppress focal-specific warnings #159

Merged
merged 7 commits into from
Aug 28, 2020
Merged

Suppress focal-specific warnings #159

merged 7 commits into from
Aug 28, 2020

Conversation

mjcarroll
Copy link
Contributor

Addresses #157 and fixes #150.

Both homebrew and Ubuntu Focal package a cppzmq version that is reported as 4.7.0, but the deprecation warnings and available functions are different. I believe this is due in part to there not being an official 4.7.0 release.

This simply detects if we are building on Ubuntu Focal and adjusts accordingly.

Signed-off-by: Michael Carroll [email protected]

@mjcarroll mjcarroll requested a review from caguero as a code owner August 6, 2020 19:35
@github-actions github-actions bot added the 🏰 citadel Ignition Citadel label Aug 6, 2020
@mjcarroll mjcarroll force-pushed the focal_warnings branch 2 times, most recently from 1e532e0 to 38d6421 Compare August 6, 2020 19:41
@mjcarroll mjcarroll changed the base branch from ign-transport8 to chapulina/7_to_8 August 6, 2020 19:41
@codecov
Copy link

codecov bot commented Aug 6, 2020

Codecov Report

Merging #159 into chapulina/7_to_8 will decrease coverage by 0.11%.
The diff coverage is n/a.

Impacted file tree graph

@@                 Coverage Diff                  @@
##           chapulina/7_to_8     #159      +/-   ##
====================================================
- Coverage             83.75%   83.64%   -0.12%     
====================================================
  Files                    49       49              
  Lines                  4832     4903      +71     
====================================================
+ Hits                   4047     4101      +54     
- Misses                  785      802      +17     
Impacted Files Coverage Δ
src/Node.cc 92.34% <ø> (+0.02%) ⬆️
src/NodeShared.cc 79.97% <ø> (-0.65%) ⬇️
include/ignition/transport/Discovery.hh 86.66% <0.00%> (+0.26%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a8308c7...840ae27. Read the comment docs.

Copy link
Contributor

@chapulina chapulina left a comment

Choose a reason for hiding this comment

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

Not sure why the DCO checker went crazy there 🧐

CMakeLists.txt Outdated Show resolved Hide resolved
src/Node.cc Show resolved Hide resolved
@caguero caguero mentioned this pull request Aug 10, 2020
@mjcarroll mjcarroll force-pushed the focal_warnings branch 2 times, most recently from 64a5fe0 to d1b848a Compare August 14, 2020 19:12
Signed-off-by: Michael Carroll <[email protected]>
mjcarroll and others added 3 commits August 28, 2020 10:02
@chapulina
Copy link
Contributor

The Log_TEST failed on Focal, not sure if it could be caused by this PR.

@mjcarroll
Copy link
Contributor Author

The Log_TEST failed on Focal, not sure if it could be caused by this PR.

It happens locally for me, as well. I think because this test doesn't involve zmq, it's more likely a difference between bionic and focal library versions (possibly sqlite?)

https://github.com/ignitionrobotics/ign-transport/blob/ign-transport8/log/src/Log_TEST.cc#L250-L259

Signed-off-by: Nate Koenig <[email protected]>
@nkoenig
Copy link
Contributor

nkoenig commented Aug 28, 2020

The Log_TEST failed on Focal, not sure if it could be caused by this PR.

It happens locally for me, as well. I think because this test doesn't involve zmq, it's more likely a difference between bionic and focal library versions (possibly sqlite?)

https://github.com/ignitionrobotics/ign-transport/blob/ign-transport8/log/src/Log_TEST.cc#L250-L259

Focal outputs a value of 7254000000ns. I'll try adding an #ifdef.

Nate Koenig and others added 2 commits August 28, 2020 13:23
Signed-off-by: Nate Koenig <[email protected]>
Signed-off-by: Carlos Agüero <[email protected]>
@caguero
Copy link
Collaborator

caguero commented Aug 28, 2020

The Log_TEST failed on Focal, not sure if it could be caused by this PR.

It happens locally for me, as well. I think because this test doesn't involve zmq, it's more likely a difference between bionic and focal library versions (possibly sqlite?)
https://github.com/ignitionrobotics/ign-transport/blob/ign-transport8/log/src/Log_TEST.cc#L250-L259

Focal outputs a value of 7254000000ns. I'll try adding an #ifdef.

I relaxed the expectation for not having to go back to this test (hopefully) after another Ubuntu release.

@caguero caguero merged commit e3f8002 into chapulina/7_to_8 Aug 28, 2020
@caguero caguero deleted the focal_warnings branch August 28, 2020 21:13
nkoenig added a commit that referenced this pull request Aug 28, 2020
* Remove warnings using ZMQ 4.3.1 or greater.
* Do not use ZMQ_CPP11
* Win debugging.
* backport improved compiler support for std::filesystem
* Close branch backport_compiler_filesystem
* Restore original Playback::Start and add overload with new parameter to fix ABI
* bump to 7.2.2 and update changelog

* Close branch fix_abi_7

* Write to disk from a background thread in log recorder

* Update Changelog

* Move `dataWriterState = true` to Recorder::Implementation::DataWriterThread() thread.

* Revert moving dataWriterState

* Failing test with incorrect time stamps

* Correctly record message reception time stamp

* Reorder functions

* Specify buffer size in MB rather than number of elements in the data queue

* Flush any remaining data to log file when stopping the Recorder

* Codecheck. The rvalue ref is used to ensure that std::vector is always moved

* Version update

* Added tag ignition-transport7_7.3.0~pre1 for changeset 173fae6c362d

* recorder.cc: include <optional>

* Add console message to indicate buffer being flushed

* Add <numeric>

* Close branch async_recorder

* Prepare for 7.3.0

* Added tag ignition-transport7_7.3.0 for changeset 367d4f1bfcf7

* Configurable buffer sizes.

* Fix typo.

* Changelog.

* Clarify high water mark policy.

* Tweak documentation and error messages.

* Close branch issue_116_transport7

* fix line lengths

* Close branch codecheck7

* Update default values for the high water marks

* Update buffer default values

* Changelog.md edited online with Bitbucket

* Close branch default_hwm

* Adding connection message.

* ConnectionMsg implementation.

* Test

* No control socket.

* Preserve ABI.

* Discard registrations when needed.

* Tweaks.

* Changelog

* Fix issue #114.

* Close branch discovery_extended_p2

* 7.4.0

* Move changelog entry

* Close branch ign-transport7-4

* Added tag ignition-transport7_7.4.0 for changeset 083e7bf41080

* Protobuf warnings

* Close branch proto_deprecations

* Close branch issue_111

* Windows warnings

* revert commit to release branch

* Fix version for send_falgs command

* Close branch ign-transport7_fix_send_flags

* Backport pull request #441

* updates

* Added another check

* Close branch issue_118

* mv hgignore

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

* add gitignore

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

* [ign-transport7] Update BitBucket links (#123)

* [ign-transport7] Update BitBucket links

Signed-off-by: Louise Poubel <[email protected]>

* changelog pull-requests

* Apply suggestions from code review

* Update tutorials/07_relay.md

Co-authored-by: Marya Belanger <[email protected]>

* [ign-transport7] Workflow updates (#132)

* [ign-transport7] Workflow updates

Signed-off-by: Louise Poubel <[email protected]>

* Helper function to get a valid topic name (#153)

Signed-off-by: Louise Poubel <[email protected]>

* Remove Windows warnings (#151)

Signed-off-by: Carlos Aguero <[email protected]>

* Remove warnings on Homebrew (#150)

Signed-off-by: Carlos Aguero <[email protected]>
Co-authored-by: Louise Poubel <[email protected]>

* Bump to 7.5.0 (#156)

Signed-off-by: Louise Poubel <[email protected]>

* Modernize actions CI (#158)

Signed-off-by: Louise Poubel <[email protected]>

* remove ci-bionic

Signed-off-by: Louise Poubel <[email protected]>

* add focal

Signed-off-by: Louise Poubel <[email protected]>

* msgs5

Signed-off-by: Louise Poubel <[email protected]>

* Suppress focal-specific warnings (#159)

* Suppress focal-specific warnings

Signed-off-by: Michael Carroll <[email protected]>

* Warn when lsb_release isn't present

Signed-off-by: Michael Carroll <[email protected]>

* Adding header guard.

Signed-off-by: Carlos Agüero <[email protected]>

* Include correct header file for version check

Signed-off-by: Michael Carroll <[email protected]>

* Added more debug output

Signed-off-by: Nate Koenig <[email protected]>

* Fix focal test and codecheck

Signed-off-by: Nate Koenig <[email protected]>

* Change endtime expectation

Signed-off-by: Carlos Agüero <[email protected]>

Co-authored-by: Carlos Agüero <[email protected]>
Co-authored-by: Nate Koenig <[email protected]>

Co-authored-by: Carlos Aguero <[email protected]>
Co-authored-by: Steve Peters <[email protected]>
Co-authored-by: Steve Peters <[email protected]>
Co-authored-by: Carlos Agüero <[email protected]>
Co-authored-by: Addisu Z. Taddese <[email protected]>
Co-authored-by: Nate Koenig <[email protected]>
Co-authored-by: Carlos Aguero <[email protected]>
Co-authored-by: Jose Luis Rivero <[email protected]>
Co-authored-by: claireyywang <[email protected]>
Co-authored-by: Marya Belanger <[email protected]>
Co-authored-by: Michael Carroll <[email protected]>
Co-authored-by: Nate Koenig <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏰 citadel Ignition Citadel
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants