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

Improved licencse matching #358

Merged
merged 5 commits into from
Jun 13, 2022

Conversation

zflat
Copy link
Contributor

@zflat zflat commented Mar 7, 2022

I am using the approach described in https://answers.ros.org/question/343351/whats-the-proper-way-to-use-a-proprietary-software-license-according-to-ament_copyright/?answer=343355#post-id-343355 to add a proprietary license for my organization that can be enforced with ament_copyright. One issue I am running into is that the license header has multiple references to a copyright statement so the current license matching fails.

I would like to update the license matching to split the license header into sections separated by {copyright_holder} or {copyright}. The current matching only splits into sections by {coyright_holder}.

Here is an abbreviated example of the license header template I need this to work with:

 {copyright}
 ALL RIGHTS RESERVED
 
 
 THIS UNPUBLISHED WORK BY {copyright_holder} IS PROTECTED. IF PUBLICATION OF THE WORK SHOULD
 OCCUR, THE FOLLOWING NOTICE SHALL APPLY.
 
  "{copyright} ALL RIGHTS RESERVED."
 
 {copyright_holder} DISCLAIMS ALL WARRANTIES WITH REGARD TO THIS SOFTWARE.

Copy link
Contributor

@audrow audrow left a comment

Choose a reason for hiding this comment

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

Looks okay to me. Would you add tests to confirm that this is working as expected.

Also, I ran CI, just to see if anything breaks downstream:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Windows Build Status

@zflat
Copy link
Contributor Author

zflat commented Mar 31, 2022

@audrow thanks for the review! The branch in #363 adds ament_copyright/test/test_parser.py which is where I would like to put unit tests for this change. Once that #363 is merged I can rebase and add tests to this branch.

@audrow
Copy link
Contributor

audrow commented Mar 31, 2022

@audrow thanks for the review! The branch in #363 adds ament_copyright/test/test_parser.py which is where I would like to put unit tests for this change. Once that #363 is merged I can rebase and add tests to this branch.

Sounds good, @zflat.

By the way, it looks like there are some code style errors in CI: https://ci.ros2.org/job/ci_linux/16480/testReport/junit/ament_copyright.test/test_flake8/test_flake8/.

@clalancette clalancette added the more-information-needed Further information is required label Apr 21, 2022
@zflat
Copy link
Contributor Author

zflat commented May 2, 2022

@clalancette I addressed the code styling with this change: 15d5d7e

I see the more-information-needed label, so does this information answer the open question?

@audrow
Copy link
Contributor

audrow commented May 2, 2022

I think the more information needed tag refers to that this is waiting on the tests to be implemented, once the other PR is merged in.

William Wedler added 3 commits June 3, 2022 15:46
Signed-off-by: William Wedler <william.wedler@resquared.com>
Signed-off-by: William Wedler <william.wedler@resquared.com>
Signed-off-by: William Wedler <william.wedler@resquared.com>
@zflat zflat force-pushed the improved_licencse_matching branch from 15d5d7e to 5c72d14 Compare June 7, 2022 23:40
Signed-off-by: William Wedler <william.wedler@resquared.com>
@zflat zflat force-pushed the improved_licencse_matching branch from 5c72d14 to be3120b Compare June 7, 2022 23:42
Signed-off-by: William Wedler <william.wedler@resquared.com>
@zflat
Copy link
Contributor Author

zflat commented Jun 8, 2022

@audrow @methylDragon I added test coverage for the changes in this pull request. Can I please get a re-review?

@methylDragon
Copy link
Contributor

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Windows Build Status

@zflat
Copy link
Contributor Author

zflat commented Jun 12, 2022

@methylDragon Thanks for kicking off CI! Unfortunately, I think this needs a new build because kdl_parse failed to build?

21:36:01 Summary: 142 packages finished [8min 44s]
21:36:01   1 package failed: kdl_parser
21:36:01   3 packages aborted: orocos_kdl rosidl_generator_py rviz_ogre_vendor
21:36:01   38 packages had stderr output: ament_clang_format ament_clang_tidy ament_copyright ament_cppcheck ament_cpplint ament_flake8 ament_index_python ament_lint ament_lint_cmake ament_mypy ament_package ament_pclint ament_pep257 ament_pycodestyle ament_pyflakes ament_uncrustify ament_xmllint domain_coordinator foonathan_memory_vendor google_benchmark_vendor iceoryx_posh ignition_cmake2_vendor ignition_math6_vendor kdl_parser launch launch_pytest launch_testing launch_xml launch_yaml libyaml_vendor mimick_vendor osrf_pycommon pybind11_vendor rosidl_cli rpyutils tracetools_read tracetools_trace zstd_vendor
21:36:01   187 packages not processed

@methylDragon
Copy link
Contributor

Oof

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Windows Build Status

Copy link
Contributor

@methylDragon methylDragon left a comment

Choose a reason for hiding this comment

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

@methylDragon methylDragon merged commit 6a155a1 into ament:master Jun 13, 2022
@zflat zflat deleted the improved_licencse_matching branch June 13, 2022 13:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
more-information-needed Further information is required
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants