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

nmos-cpp: new release and conan v2 migration #14152

Closed
wants to merge 22 commits into from

Conversation

garethsb
Copy link
Contributor

@garethsb garethsb commented Nov 11, 2022

Specify library name and version: nmos-cpp/cci.20221124


  • I've read the guidelines for contributing.
  • I've followed the PEP8 style guides for Python code in the recipes.
  • I've used the latest Conan client version.
  • I've tried at least one configuration locally with the conan-center hook activated.

@conan-center-bot

This comment has been minimized.

@garethsb
Copy link
Contributor Author

Waiting for #13959 and #13962.

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@garethsb
Copy link
Contributor Author

garethsb commented Nov 14, 2022

This is still waiting for #13959 and #13962 in order to have any chance of building on CCI, but locally, I'm just struggling with the paths to the packaged executables, which means that the test package doesn't run successfully.

@samuel-emrys This is the PATH issue that we discussed on Slack.

I attempted a few different ways of adding the right directory in package_info():

The recipe's existing approach:

        self.env_info.PATH.append(bin_path)

seems to have no effect on the generated conanrunenv-release-x86_64.bat. Understandable I think as env_info is the "old" way.

However, my first attempt:

        self.cpp_info.bindirs = [bindir]
        self.cpp_info.libdirs = [libdir]

also seem to have no effect on that file. The default bin directory is prepended to the PATH, not my specified bindir...

My next attempt (updated after realising I needed append_path rather than plain append):

        self.runenv_info.prepend_path("PATH", bin_path)

works OK.

However, the test package can't run successfully, because the test package uses subprocess.Popen as well as self.run. I've updated the self.run call to use env="conanrun", but I think I need to get the variables from the VirtualRunEnv for the call to subprocess.Popen(..., env=???) and I can't work out the syntax for that.

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@garethsb
Copy link
Contributor Author

Now broken by issue discussed in #14223.

@conan-center-bot

This comment has been minimized.

@garethsb garethsb force-pushed the nmos-cpp-20221111 branch 2 times, most recently from 627e102 to 41fb3b1 Compare November 16, 2022 22:19
@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

Preemptive fix for mdnsresponder recipe change to cmake package/target name
@conan-center-bot

This comment has been minimized.

@garethsb
Copy link
Contributor Author

Sigh... 🪲 Macos, x86_64, apple-clang, 12.0, libc++, Release....

ERROR: nmos-cpp/cci.20220120 (test package): Error in test() method, line 44
	self.run(bin_path + " node-config.json", env="conanrun", output=node_out)
	ConanException: Error 138 while executing ./test_package node-config.json

(cherry picked from commit 69d10fc)
@conan-center-bot

This comment has been minimized.

Co-authored-by: Uilian Ries <uilianries@gmail.com>
@conan-center-bot

This comment has been minimized.

Copy link
Contributor

@prince-chrismc prince-chrismc left a comment

Choose a reason for hiding this comment

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

3 tiny comments

  • source for the patches
  • nicer comment
  • more details for the patches (our goal is to have smaller patches so the extra comments need a bit more explaining)

@conan-center-bot

This comment has been minimized.

uilianries
uilianries previously approved these changes Nov 29, 2022
Copy link
Member

@uilianries uilianries left a comment

Choose a reason for hiding this comment

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

LGTM

@prince-chrismc
Copy link
Contributor

Sorry for dismissing the review 🙊 I just wanted to make sure the linters worked correctly

@garethsb
Copy link
Contributor Author

garethsb commented Nov 29, 2022

Sorry for dismissing the review 🙊 I just wanted to make sure the linters worked correctly

I copied that list format from another CCI recipe too 🙄

If https://github.com/conan-io/conan-center-index/blob/97b198659f0b6ac39b259a4d99232c1ac725d629/docs/adding_packages/conandata_yml_format.md#patch_source constitutes the spec, I'm not convinced the multiline format is any more valid than the list... 😖

@conan-center-bot
Copy link
Collaborator

Conan v1 pipeline

All green in build 28 (4e0dcd822dadc1ed3de69d5984b93052857efcf9):

  • nmos-cpp/cci.20221124@:
    All packages built successfully! (All logs)

  • nmos-cpp/cci.20220208@:
    All packages built successfully! (All logs)

  • nmos-cpp/cci.20220620@:
    All packages built successfully! (All logs)

  • nmos-cpp/cci.20220428@:
    All packages built successfully! (All logs)

  • nmos-cpp/cci.20220120@:
    All packages built successfully! (All logs)

@garethsb
Copy link
Contributor Author

Withdrawing this PR. Will clean it up and submit again. Thanks for all reviews and assistance so far @uilianries, @prince-chrismc.

@garethsb garethsb closed this Nov 30, 2022
@prince-chrismc
Copy link
Contributor

If https://github.com/conan-io/conan-center-index/blob/97b198659f0b6ac39b259a4d99232c1ac725d629/docs/adding_packages/conandata_yml_format.md#patch_source constitutes the spec, I'm not convinced the multiline format is any more valid than the list... 😖

Optional("patch_source"): Str(),
:)

This is only for humans to read and there's not really a way to make it "machine readable" that I could image so I did not make it more complicated 🤷 I understand it's not intuitive but I am pretty sure I need to redo that linter in the new year

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.

4 participants