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

Generate Drake exports from CPS #6014

Conversation

mwoehlke-kitware
Copy link
Contributor

@mwoehlke-kitware mwoehlke-kitware commented May 3, 2017

Clean up mechanism for wholesale file generation in Bazel. Add PyCPS and dependency semantic_version. Move generation of drake-config.cmake via drake.cps from an offline step to a Bazel build rule.

Besides that generating files during the build rather than offline is better anyway, this will also help streamline things when stuff in the CPS changes, as it will no longer be necessary to manually regenerate drake-config.cmake.


This change is Reviewable

@jwnimmer-tri jwnimmer-tri self-assigned this May 3, 2017
@jwnimmer-tri jwnimmer-tri removed their request for review May 3, 2017 14:57
@jwnimmer-tri
Copy link
Collaborator

I will look late tonight or first thing tomorrow.

@mwoehlke-kitware mwoehlke-kitware force-pushed the generate-drake-config-from-cps branch from cbd6cce to b83b6c9 Compare May 3, 2017 15:05
@jamiesnape
Copy link
Contributor

Review status: 0 of 9 files reviewed at latest revision, 4 unresolved discussions.


WORKSPACE, line 298 at r1 (raw file):

    commit = "v2.6.0",
    sha256 = "110d9c75dc9670a188ab820503c6b40ea4ab3f1450a0aee5a90a24fd60aef358",
    build_file = "tools/semantic_version.BUILD",

Use the PyPI version:

    new_http_archive(
        name = "semantic_version",
        urls = [
          "https://pypi.python.org/packages/source/s/semantic_version/semantic_version-2.6.0.tar.gz",
          "https://d2tbce6hkathzp.cloudfront.net/pypi/semantic_version/semantic_version-2.6.0.tar.gz",
          "https://s3.amazonaws.com/drake-mirror/pypi/semantic_version/semantic_version-2.6.0.tar.gz",
        ]
        sha256 = "2a4328680073e9b243667b201119772aefc5fc63ae32398d6afafff07c4f54c0",
        build_file = "tools/semantic_version.BUILD",
        strip_prefix = "semantic_version-2.6.0",
   )

WORKSPACE, line 307 at r1 (raw file):

    sha256 = "314cfbb3b2bf812bbbc9759171a91049a9f181a8aad90c258c182aa9be5eb4e6",
    build_file = "tools/pycps.BUILD",
)

This needs a strip_prefix too, but probably github_archive needs tweaking to use it.


tools/semantic_version.BUILD, line 3 at r1 (raw file):

load("@//tools:drake.bzl", "drake_generate_file")

drake_generate_file(

The strip_prefix in the WORKSPACE file should make this redundant.


tools/semantic_version.BUILD, line 11 at r1 (raw file):

py_library(
  name = "semantic_version",
  srcs = glob(["semantic_version/*.py"]) + ["__init__.py"],

Remove + ["__init__.py"].


Comments from Reviewable

@jamiesnape
Copy link
Contributor

Review status: 0 of 9 files reviewed at latest revision, 7 unresolved discussions.


tools/pycps.BUILD, line 9 at r1 (raw file):


py_library(
  name = "pycps",

Probably should be named cps.


tools/pycps.BUILD, line 11 at r1 (raw file):

  name = "pycps",
  srcs = ["cps.py"],
  deps = ["@semantic_version//:semantic_version"]

,


tools/pycps.BUILD, line 15 at r1 (raw file):


py_binary(
  name = "cps2cmake_executable",

Should be named cps2cmake by convention.


Comments from Reviewable

@jamiesnape
Copy link
Contributor

Review status: 0 of 9 files reviewed at latest revision, 7 unresolved discussions.


WORKSPACE, line 298 at r1 (raw file):

urls = [

Quoted 4 lines of code… > "https://pypi.python.org/packages/source/s/semantic_version/semantic_version-2.6.0.tar.gz", > "https://d2tbce6hkathzp.cloudfront.net/pypi/semantic_version/semantic_version-2.6.0.tar.gz", > "https://s3.amazonaws.com/drake-mirror/pypi/semantic_version/semantic_version-2.6.0.tar.gz", > ]
urls = [
          "https://pypi.python.org/packages/source/s/semantic_version/semantic_version-2.6.0.tar.gz",
          "https://d2tbce6hkathzp.cloudfront.net/pypi/semantic_version/semantic_version-2.6.0.tar.gz",
          "https://s3.amazonaws.com/drake-mirror/pypi/semantic_version/semantic_version-2.6.0.tar.gz",
        ],

Comments from Reviewable

@jamiesnape jamiesnape self-assigned this May 3, 2017
@mwoehlke-kitware
Copy link
Contributor Author

Review status: 0 of 9 files reviewed at latest revision, 7 unresolved discussions.


WORKSPACE, line 298 at r1 (raw file):

Previously, jamiesnape (Jamie Snape) wrote…

urls = [
"https://pypi.python.org/packages/source/s/semantic_version/semantic_version-2.6.0.tar.gz",
"https://d2tbce6hkathzp.cloudfront.net/pypi/semantic_version/semantic_version-2.6.0.tar.gz",
"https://s3.amazonaws.com/drake-mirror/pypi/semantic_version/semantic_version-2.6.0.tar.gz",
]

urls = [
          "https://pypi.python.org/packages/source/s/semantic_version/semantic_version-2.6.0.tar.gz",
          "https://d2tbce6hkathzp.cloudfront.net/pypi/semantic_version/semantic_version-2.6.0.tar.gz",
          "https://s3.amazonaws.com/drake-mirror/pypi/semantic_version/semantic_version-2.6.0.tar.gz",
        ],

Why? What is the advantage of PyPI vs. upstream?


WORKSPACE, line 307 at r1 (raw file):

Previously, jamiesnape (Jamie Snape) wrote…

This needs a strip_prefix too, but probably github_archive needs tweaking to use it.

Um... why? It seems to be Doing The Right Thing without it...


tools/pycps.BUILD, line 15 at r1 (raw file):

Previously, jamiesnape (Jamie Snape) wrote…

Should be named cps2cmake by convention.

Can't; conflicts with the source file. This is (IMHO) really, really ugly, but AFAICT Bazel forces me to add some junk to the target name to disambiguate.

If there's a better way to export a source file that is already an executable script (that also needs dependencies), I'd love to know! (Note: at least one person on bazel-discuss already implied that there is not... 😢)


tools/semantic_version.BUILD, line 11 at r1 (raw file):

Previously, jamiesnape (Jamie Snape) wrote…

Remove + ["__init__.py"].

Is it picked up automatically?


Comments from Reviewable

@mwoehlke-kitware
Copy link
Contributor Author

Review status: 0 of 9 files reviewed at latest revision, 7 unresolved discussions.


tools/pycps.BUILD, line 9 at r1 (raw file):

Previously, jamiesnape (Jamie Snape) wrote…

Probably should be named cps.

Done.


tools/pycps.BUILD, line 11 at r1 (raw file):

Previously, jamiesnape (Jamie Snape) wrote…

,

Done.


Comments from Reviewable

@mwoehlke-kitware mwoehlke-kitware force-pushed the generate-drake-config-from-cps branch from b83b6c9 to 2997a8f Compare May 3, 2017 17:38
@jamiesnape
Copy link
Contributor

Review status: 0 of 9 files reviewed at latest revision, 5 unresolved discussions.


WORKSPACE, line 298 at r1 (raw file):

Previously, mwoehlke-kitware (Matthew Woehlke) wrote…

Why? What is the advantage of PyPI vs. upstream?

It is consistent with the other Python packages that we use (and I already mirrored the PyPI version).


WORKSPACE, line 307 at r1 (raw file):

Previously, mwoehlke-kitware (Matthew Woehlke) wrote…

Um... why? It seems to be Doing The Right Thing without it...

It see, you are copying to change the file extension. Nevermind.


tools/semantic_version.BUILD, line 11 at r1 (raw file):

Previously, mwoehlke-kitware (Matthew Woehlke) wrote…

Is it picked up automatically?

That level will be removed. If it were not, you would add an imports = ["semantic_version-2.6.0"] line instead in any case.


Comments from Reviewable

@jamiesnape
Copy link
Contributor

Review status: 0 of 9 files reviewed at latest revision, 4 unresolved discussions.


tools/pycps.BUILD, line 15 at r1 (raw file):

Previously, mwoehlke-kitware (Matthew Woehlke) wrote…

Can't; conflicts with the source file. This is (IMHO) really, really ugly, but AFAICT Bazel forces me to add some junk to the target name to disambiguate.

If there's a better way to export a source file that is already an executable script (that also needs dependencies), I'd love to know! (Note: at least one person on bazel-discuss already implied that there is not... 😢)

Not that I can think of at the moment.


Comments from Reviewable

@mwoehlke-kitware
Copy link
Contributor Author

Review status: 0 of 9 files reviewed at latest revision, 3 unresolved discussions.


WORKSPACE, line 298 at r1 (raw file):

Previously, jamiesnape (Jamie Snape) wrote…

It is consistent with the other Python packages that we use (and I already mirrored the PyPI version).

https://pypi.python.org/packages/source/s/semantic_version/semantic_version-2.6.0.tar.gz → 404?


Comments from Reviewable

@jamiesnape
Copy link
Contributor

Review status: 0 of 9 files reviewed at latest revision, 3 unresolved discussions.


WORKSPACE, line 298 at r1 (raw file):

Previously, mwoehlke-kitware (Matthew Woehlke) wrote…

https://pypi.python.org/packages/source/s/semantic_version/semantic_version-2.6.0.tar.gz → 404?

https://files.pythonhosted.org/packages/source/s/semantic_version/semantic_version-2.6.0.tar.gz, I guess. Read https://bitbucket.org/pypa/pypi/issues/438/backwards-compatible-un-hashed-package for why.


Comments from Reviewable

@jwnimmer-tri
Copy link
Collaborator

Reviewed 8 of 9 files at r1, 1 of 1 files at r2.
Review status: all files reviewed at latest revision, 5 unresolved discussions.


WORKSPACE, line 302 at r2 (raw file):

github_archive(
    name = "pycps",

pycps is GPL. That is unacceptable for a dependency of Drake's core software.

We could probably live with LGPL 2+ or 3+, but really file-only-viral (MPL2) or non-viral would be best if possible.

It would also be a bonus if the tool's documentation explicitly disclaimed copyright on its outputs.


tools/pycps.BUILD, line 6 at r2 (raw file):

  outs = ["cps2cmake.py"],
  cmd = "cp \"$<\" \"$(@)\"",
)

FYI Does this work instead?

alias(
    name = "cps2cmake.py",
    actual = ":cps2cmake",
)

(I assume you already tried cps2cmake bare as the py_binary's srcs and main and it failed because of the missing extension.)


Comments from Reviewable

Add new helper drake_generate_file to generate a file with specified
content. Use this instead of a genrule invoking echo to generate a file
for LCM.
@mwoehlke-kitware mwoehlke-kitware force-pushed the generate-drake-config-from-cps branch from 2997a8f to f6b1290 Compare May 4, 2017 16:04
@mwoehlke-kitware
Copy link
Contributor Author

Review status: 2 of 9 files reviewed at latest revision, 5 unresolved discussions.


WORKSPACE, line 298 at r1 (raw file):

Previously, jamiesnape (Jamie Snape) wrote…

https://files.pythonhosted.org/packages/source/s/semantic_version/semantic_version-2.6.0.tar.gz, I guess. Read https://bitbucket.org/pypa/pypi/issues/438/backwards-compatible-un-hashed-package for why.

Done.


WORKSPACE, line 302 at r2 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

pycps is GPL. That is unacceptable for a dependency of Drake's core software.

We could probably live with LGPL 2+ or 3+, but really file-only-viral (MPL2) or non-viral would be best if possible.

It would also be a bonus if the tool's documentation explicitly disclaimed copyright on its outputs.

This shouldn't be a problem, any more than building Drake with GCC is a problem. We are not incorporating any code from PyCPS, and it is widely understood that the output of a GPL tool is not itself subject to GPL. (Also, for the record, the author hereby says as much.)

I'm not aware of any precedent for such disclaimers (e.g. this wasn't a problem when drake-config.cmake was generated by CMake). Do you have any recommendations?

(All that said, it does make sense on further reflection for PyCPS to be LGPL, but I don't think that should necessarily block this PR.)


tools/semantic_version.BUILD, line 3 at r1 (raw file):

Previously, jamiesnape (Jamie Snape) wrote…

The strip_prefix in the WORKSPACE file should make this redundant.

Done.


tools/semantic_version.BUILD, line 11 at r1 (raw file):

Previously, jamiesnape (Jamie Snape) wrote…

That level will be removed. If it were not, you would add an imports = ["semantic_version-2.6.0"] line instead in any case.

Done.


Comments from Reviewable

@mwoehlke-kitware
Copy link
Contributor Author

Review status: 2 of 9 files reviewed at latest revision, 5 unresolved discussions.


WORKSPACE, line 302 at r2 (raw file):
I added this to the README:

It is generally recognized that the outputs of a software tool are not derivative works of that tool unless they contain substantial and creative parts of the tool's code. The author(s) explicitly affirm that the outputs of the PyCPS tools are not derivative works of PyCPS, and are accordingly not subject to the terms of PyCPS's license. Such outputs may, however, be derivative works of those CPS files which PyCPS processes as inputs.


tools/pycps.BUILD, line 6 at r2 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

FYI Does this work instead?

alias(
    name = "cps2cmake.py",
    actual = ":cps2cmake",
)

(I assume you already tried cps2cmake bare as the py_binary's srcs and main and it failed because of the missing extension.)

Nope:

ERROR: .../external/pycps/BUILD.bazel:15:10:
  in main attribute of py_binary rule @pycps//:cps2cmake_executable:
  file '@pycps//:cps2cmake' (aliased through '@pycps//:cps2cmake.py') is misplaced here (expected .py).

py_binary is evil fussy 😦, and alas, sees right through the attempt to pacify it.

In my opinion, the "right" thing to do here is to somehow just tell Bazel "hey, this source file is a Python script, it has some (run-time) dependencies but you don't need to do anything special with it". Alas, I don't know how to do that, and @laszlocsomor says this ugly hoop-jumping is the "right" thing to do. (FWIW, I'm more bothered that I can't use the script's real name as the Bazel target name 😢.)


Comments from Reviewable

@mwoehlke-kitware
Copy link
Contributor Author

@drake-jenkins-bot retest this please

@jamiesnape
Copy link
Contributor

Please update #4045 if/when this merges.

@jwnimmer-tri
Copy link
Collaborator

:lgtm:


Reviewed 2 of 7 files at r3, 4 of 4 files at r4, 1 of 1 files at r5.
Review status: all files reviewed at latest revision, 3 unresolved discussions.


WORKSPACE, line 302 at r2 (raw file):

Previously, mwoehlke-kitware (Matthew Woehlke) wrote…

I added this to the README:

It is generally recognized that the outputs of a software tool are not derivative works of that tool unless they contain substantial and creative parts of the tool's code. The author(s) explicitly affirm that the outputs of the PyCPS tools are not derivative works of PyCPS, and are accordingly not subject to the terms of PyCPS's license. Such outputs may, however, be derivative works of those CPS files which PyCPS processes as inputs.

OK I will take a note to think about this further, and allow it for now. I would be slightly happier if cps2cmake_executable just listed all sources, and there we no py_library that a Drake developer could accidentally link to. (Or maybe just add a comment in the py_library about its private visibility being really important.)


Comments from Reviewable

@mwoehlke-kitware
Copy link
Contributor Author

Review status: 8 of 9 files reviewed at latest revision, 3 unresolved discussions.


WORKSPACE, line 302 at r2 (raw file):

I would be slightly happier if cps2cmake_executable just listed all sources [...]

I started to do that, but it just rubs me wrong 😄. Conceptually, PyCPS consists of a) the cps Python module, and b) some CLI tools that make use of the same (currently just cmake2cps, but others are planned, eventually). Ergo...

Or maybe just add a comment in the py_library about its private visibility being really important.

...I went with this. (It's private anyway, with no expectation that it would ever be used at other than build time in the worst case, but if it helps you feel better, I'm more than happy adding a comment why we want it private.)


Comments from Reviewable

@jamiesnape
Copy link
Contributor

:lgtm:


Reviewed 1 of 9 files at r1, 2 of 7 files at r3, 4 of 4 files at r4, 1 of 1 files at r5, 1 of 1 files at r6.
Review status: all files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@jwnimmer-tri
Copy link
Collaborator

+(status: curate commits before merging)


Reviewed 1 of 1 files at r6.
Review status: all files reviewed at latest revision, all discussions resolved, some commit checks failed.


Comments from Reviewable

Add PyCPS as a new Bazel external, along with its dependency, (the
Python module) semantic_version. This will allow us to generate
drake-config.cmake from drake.cps as part of the build process, rather
than doing it manually.

(This is not needed for the CMake build, since that generates
drake-config.cmake differently.)
Remove the hand-generated drake-config.cmake and replace with a rule to
generate it from drake.cps at build time. Update packaging accordingly.
@mwoehlke-kitware mwoehlke-kitware force-pushed the generate-drake-config-from-cps branch from 37956bd to 34ac102 Compare May 4, 2017 20:34
@stonier
Copy link
Contributor

stonier commented May 4, 2017

WORKSPACE, line 302 at r2 (raw file):

Previously, mwoehlke-kitware (Matthew Woehlke) wrote…

I would be slightly happier if cps2cmake_executable just listed all sources [...]

I started to do that, but it just rubs me wrong 😄. Conceptually, PyCPS consists of a) the cps Python module, and b) some CLI tools that make use of the same (currently just cmake2cps, but others are planned, eventually). Ergo...

Or maybe just add a comment in the py_library about its private visibility being really important.

...I went with this. (It's private anyway, with no expectation that it would ever be used at other than build time in the worst case, but if it helps you feel better, I'm more than happy adding a comment why we want it private.)

The GPL FAQ explicitly states that a GPL program cannot affect the output of a program. Basically what goes in, must come out. Just to be explicit:

https://www.gnu.org/licenses/gpl-faq.en.html#WhatCaseIsOutputGPL

So it really shouldn't be a problem here given that it's use is to generate drake-config.cmake from the drake repository files.

Nonetheless, nice to see it go LGPL and I like the private visibility (learn something new about bazel every day :)).


Comments from Reviewable

@jwnimmer-tri
Copy link
Collaborator

Yup. This is good. In my earlier review, I'd only considered the "Everything in WORKSPACE under platform review must pass a licensing cross-check", but in this particular context the rules are different and I hadn't considered the tool output exception.

@laszlocsomor
Copy link

@mwoehlke-kitware : re:

In my opinion, the "right" thing to do here is to somehow just tell Bazel "hey, this source file is a Python script, it has some (run-time) dependencies but you don't need to do anything special with it". Alas, I don't know how to do that, and @laszlocsomor says this ugly hoop-jumping is the "right" thing to do.

After reading this I thought more about the problem and realized, if all you want is to set the runtime dependencies of the python script but otherwise don't care about anything the py_* rules would add (compilation, type checking, whatever they might do, honestly I don't know for sure), then you can just create a sh_binary instead, it's less picky than the py_binary.

Here, "foo" is the python script, "shbin" can be built, "pybin" cannot:

sh_binary(
    name = "shbin",
    srcs = ["foo"],
    data = [...],
)

py_binary(
    name = "pybin",
    srcs = ["foo"],
    data = [...],
)

@mwoehlke-kitware
Copy link
Contributor Author

@laszlocsomor, thanks for looking again. Alas, while I indeed don't care about compiling or whatnot, I do care about being able to bring in py_librarys, as even best case, the script depends on an external Python module (namely semantic_version, as can be seen in the patch). It seems sh_binary doesn't like Python libraries as deps.

@mwoehlke-kitware mwoehlke-kitware merged commit dc9af67 into RobotLocomotion:master May 5, 2017
@mwoehlke-kitware mwoehlke-kitware deleted the generate-drake-config-from-cps branch May 5, 2017 13:57
@jamiesnape
Copy link
Contributor

jamiesnape commented May 5, 2017

It seems sh_binary doesn't like Python libraries as deps.

You would have to specify it as data in this case.

@mwoehlke-kitware
Copy link
Contributor Author

You would have to specify it as data in this case.

That may or may not be following transitive dependencies, and in any case, it doesn't seem to work with external py_library dependencies; even with data = ["@semantic_version//:semantic_version"], it fails to import semantic_version. (Provided I remember to dnf remove python2-semantic_version for testing purposes 😉.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants