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

Fix symlink install versioned shared library #216

Merged
merged 2 commits into from Jan 29, 2020

Conversation

JafarAbdi
Copy link
Contributor

Fix #214

From doing some online research I found that the versioning extension is just for shared libraries in Linux so the other checks don't need any change

Signed-off-by: JafarAbdi <cafer.abdi@gmail.com>
@dirk-thomas
Copy link
Contributor

Please see my comment on the original ticket about a simple wildcard to be marching to broadly. Instead use a now specific regex to only capture the intended version suffices.

@JafarAbdi
Copy link
Contributor Author

JafarAbdi commented Jan 29, 2020

@dirk-thomas Sorry for the delay again does the following regex ok .? \.so(\.[0-9]+)?(\.[0-9]+)?(\.[0-9]+)?$ I'm not an regex expert but I think this should works, see for testing

EDIT: Found a better one ^\\.so(\\.[0-9]+)*$ (Thanks to @nbbrooks) and it turned out that I have to use double-backslashs see

@dirk-thomas
Copy link
Contributor

You can simplify your regex without allowing an arbitrary amount of version parts (e.g. .1.2.3.4 shouldn't pass according to your original test examples): \.so(\.\d+){0,3}

@dirk-thomas dirk-thomas added the in progress Actively being worked on (Kanban column) label Jan 29, 2020
@nbbrooks
Copy link

If we don't want to allow multiple .#, then how about
\.so(\.\d+)?$

@JafarAbdi
Copy link
Contributor Author

@dirk-thomas CMake regex doesn't support curly brackets see

@JafarAbdi
Copy link
Contributor Author

If we don't want to allow multiple .#, then how about
\.so(\.\d+)?$

It should only pass the following extensions .so, .so.major, .so.major.minor, or .so.major.minor.patch

@nbbrooks
Copy link

Gotcha. In that case, without curly braces support I think your initial solution works best.

@JafarAbdi
Copy link
Contributor Author

Example with the last updated regex

cmake_minimum_required(VERSION 3.1.3)
project(testing_project)

set(exts
	".so.1100"
	".so.1.2.4"
	".so.10.2.4"
	".so.1.20.4"
	".so.1.2.40"
	".so.2.1"
	".so.2.1.2.1.2"
	".sos.2"
	".sos"
	".so"
	".so.1"
	".so.2.0.0"
	".so.1.s"
	"sso.1.s"
	".so.s.1"
	".so.1.2.s"
)

foreach(ext ${exts})
    if(ext MATCHES "\.so(\.[0-9]+)?(\.[0-9]+)?(\.[0-9]+)?$")
    	message("Pass: ${ext}")
    else()
    	message("Fail: ${ext}")
    endif()
endforeach()

The output

Pass: .so.1100
Pass: .so.1.2.4
Pass: .so.10.2.4
Pass: .so.1.20.4
Pass: .so.1.2.40
Pass: .so.2.1
Fail: .so.2.1.2.1.2
Fail: .sos.2
Fail: .sos
Pass: .so
Pass: .so.1
Pass: .so.2.0.0
Fail: .so.1.s
Fail: sso.1.s
Fail: .so.s.1
Fail: .so.1.2.s

Signed-off-by: JafarAbdi <cafer.abdi@gmail.com>
@JafarAbdi
Copy link
Contributor Author

Why the Fpr__ament_cmake__ubuntu_focal_amd64 check fails .?

@dirk-thomas
Copy link
Contributor

Why the Fpr__ament_cmake__ubuntu_focal_amd64 check fails .?

Unfortunately the PR testing for Foxy isn't working yet. You can ignore that status. I will trigger a set of manual CI builds once the patch is ready.

@dirk-thomas
Copy link
Contributor

CI Linux build: Build Status

Copy link
Contributor

@dirk-thomas dirk-thomas left a comment

Choose a reason for hiding this comment

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

Thanks for the improvement.

@dirk-thomas dirk-thomas merged commit 0fd5578 into ament:master Jan 29, 2020
@dirk-thomas dirk-thomas removed the in progress Actively being worked on (Kanban column) label Jan 29, 2020
@ivanpauno
Copy link
Contributor

ivanpauno commented Jan 31, 2020

I'm locally getting errors like:

CMake Warning (dev) at ament_cmake_symlink_install_targets_0_.cmake:1 (ament_cmake_symlink_install_targets):
  Syntax error in cmake code at

    /home/ivanpauno/ros2_ws/build/tracetools/ament_cmake_symlink_install/ament_cmake_symlink_install.cmake:243

  when parsing string

    \.so(\.[0-9]+)?(\.[0-9]+)?(\.[0-9]+)?$

  Invalid escape sequence \.

  Policy CMP0010 is not set: Bad variable reference syntax is an error.  Run
  "cmake --help-policy CMP0010" for policy details.  Use the cmake_policy
  command to set the policy and suppress this warning.
Call Stack (most recent call first):
  ament_cmake_symlink_install/ament_cmake_symlink_install.cmake:320 (include)
  cmake_install.cmake:41 (include)
This warning is for project developers.  Use -Wno-dev to suppress it.

Though I don't see that in the CI logs.

@JafarAbdi
Copy link
Contributor Author

@ivanpauno Does replacing it with \\.so(\.[0-9]+)?(\.[0-9]+)?(\.[0-9]+)?$ gives the error .?

@ivanpauno
Copy link
Contributor

@ivanpauno Does replacing it with \\.so(\.[0-9]+)?(\.[0-9]+)?(\.[0-9]+)?$ gives the error .?

Yeah, I've tried same error.

@JafarAbdi
Copy link
Contributor Author

@ivanpauno Could you please share the instruction to reproduce it .?

@dirk-thomas
Copy link
Contributor

See #217 for the fix.

@dirk-thomas
Copy link
Contributor

Could you please share the instruction to reproduce it

As the output above shows the CMake warning appears when building the tracetools package.

scpeters pushed a commit to scpeters/ament_cmake that referenced this pull request Nov 19, 2020
* Fix symlink install versioned shared library

Signed-off-by: JafarAbdi <cafer.abdi@gmail.com>

* Update ament_cmake_symlink_install.cmake.in

Signed-off-by: JafarAbdi <cafer.abdi@gmail.com>
scpeters pushed a commit to scpeters/ament_cmake that referenced this pull request Nov 20, 2020
* Fix symlink install versioned shared library

Signed-off-by: JafarAbdi <cafer.abdi@gmail.com>

* Update ament_cmake_symlink_install.cmake.in

Signed-off-by: JafarAbdi <cafer.abdi@gmail.com>
Signed-off-by: Steve Peters <scpeters@openrobotics.org>
hidmic pushed a commit that referenced this pull request Nov 20, 2020
* Fix symlink install versioned shared library

Signed-off-by: JafarAbdi <cafer.abdi@gmail.com>

* Update ament_cmake_symlink_install.cmake.in

Signed-off-by: JafarAbdi <cafer.abdi@gmail.com>
hidmic pushed a commit that referenced this pull request Nov 20, 2020
* Fix symlink install versioned shared library

Signed-off-by: Jafar Abdi <cafer.abdi@gmail.com>

* Update ament_cmake_symlink_install.cmake.in

Signed-off-by: Jafar Abdi <cafer.abdi@gmail.com>
jacobperron pushed a commit that referenced this pull request Nov 20, 2020
* Fix symlink install versioned shared library

Signed-off-by: JafarAbdi <cafer.abdi@gmail.com>

* Update ament_cmake_symlink_install.cmake.in

Signed-off-by: JafarAbdi <cafer.abdi@gmail.com>
Signed-off-by: Steve Peters <scpeters@openrobotics.org>
mjcarroll pushed a commit that referenced this pull request Nov 30, 2020
) (#217) (#310)

* Fix symlink install versioned shared library (#216)

* Fix symlink install versioned shared library

Signed-off-by: Jafar Abdi <cafer.abdi@gmail.com>

* Update ament_cmake_symlink_install.cmake.in

Signed-off-by: Jafar Abdi <cafer.abdi@gmail.com>

* fix escaping of regex (#217)

Signed-off-by: Dirk Thomas <dirk-thomas@users.noreply.github.com>

Co-authored-by: Jafar Abdi <cafer.abdi@gmail.com>
Co-authored-by: Dirk Thomas <dirk-thomas@users.noreply.github.com>
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.

Bug in ament_cmake_symlink_install
4 participants