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

Add preliminary test using PCL within the Bazel external project. #37

Conversation

EricCousineau-TRI
Copy link
Collaborator

@EricCousineau-TRI EricCousineau-TRI commented Sep 28, 2017

This is a simple inclusion of a custom-built PCL as a means to test a Drake-compatible PCL (for other downstream projects). This is a stop-gap, to be replaced by the system version once dependency conflicts with Drake are resolved.

This presently is only built for Xenial, and only for Bazel for simplicity.

\cc @siyuanfeng-tri @stonier

NOTE:
This will fail in Jenkins since there is not yet an HTTP archive. I will upload that soon.


This change is Reviewable

@jwnimmer-tri
Copy link
Contributor

FYI Some of the new files here say "Copyright (c) 2017, Massachusetts Institute of Technology", which seems like lies.

@EricCousineau-TRI
Copy link
Collaborator Author

True; however, the other files in this repository all follow this convention.
I'd prefer to leave it as-is to be consistent with the rest of the files (all of which have this notice), until those are changed.

@stonier Should we change these notices?

@jwnimmer-tri
Copy link
Contributor

Contributions by Jamie are plausibly copyright MIT (depending on how the Kitware contract is set up -- I don't know). Contributions TRI employees are should not be badged as owned by MIT.

@EricCousineau-TRI
Copy link
Collaborator Author

Changed the notices on the new files.

@EricCousineau-TRI EricCousineau-TRI mentioned this pull request Sep 28, 2017
5 tasks
@stonier
Copy link
Contributor

stonier commented Oct 2, 2017

@jwnimmer-tri Drake code is entirely blanketed by a single BSD w/ Copyright to the CSAIL Robot Locomotion Group. It has no individual license notices in each file. I believe @jamiesnape was using the same principle here, with the only change being that it is backed by a proper legal entity (i.e. MIT).

If contributions from TRI employees should not be badged as owned by MIT, should not this apply to Drake as well?

@jwnimmer-tri
Copy link
Contributor

jwnimmer-tri commented Oct 2, 2017

If contributions from TRI employees should not be badged as owned by MIT, should not this apply to Drake as well?

Correct, Drake is defective and I'm going to fix it RobotLocomotion/drake#1805. The difference here is that in shamb every file has many files have a license notice in each file (yay), and that notice lists the precise copyright owners (meh) instead of citing a project-wide copyright ownership file (which is what I'd recommend). If you are going to badge ownership of each file individually, I applaud the effort, but then you need to make sure its correct.

In any case, the fact that Drake is wrong and unfixed-so-far is an even better reason to fix it sooner rather than later in shamb, so that it's correct before even more wrong-copypasta happens.

@stonier
Copy link
Contributor

stonier commented Oct 2, 2017

+1 from me then.

Note: shambhala does not have a license on every file.

@jamiesnape
Copy link
Contributor

jamiesnape commented Oct 2, 2017

The intention was that shambhala has a header on every file. There are two ways of doing this, I guess. One is that copyright assignment to MIT (or another entity or group of entities) is a requirement of submitting to the repo (e.g., VTK), the other that files are allowed to have copyright assignments to third parties (e.g., CMake). We have projects that do both at Kitware. Obviously, the former is easier to administer.

@jamiesnape
Copy link
Contributor

jamiesnape commented Oct 2, 2017

(For dates, the year should be the year the file was created, per Google guidelines, hence there is a copyright notice on each file.)

@@ -1,6 +1,6 @@
#!/bin/bash

# Copyright (c) 2017, Massachusetts Institute of Technology
# Copyright (c) 2017, Toyota Research Institute
Copy link
Contributor

Choose a reason for hiding this comment

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

Whatever we decide, it is definitely not correct to remove a copyright holder when something in an existing file is changed.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Changed it back, sorry about that!

@EricCousineau-TRI
Copy link
Collaborator Author

Should I hold off until drake#1805 to move forward with this PR?
Or should I look into uploading the archive for preliminary testing, and defer fixing the notices to the resolution of the other issue?

@stonier
Copy link
Contributor

stonier commented Oct 4, 2017

Licensing less of an issue and other files have to spin up to speed as well. I'll do a PR for them to set shambhala right separate to this one.

Bigger issue right now is the CI on fire.

@stonier
Copy link
Contributor

stonier commented Oct 23, 2017

Removed official support for drake_bazel_external until either bazel (long shot) or the drake core libraries (#7529) provide support.

Do keep this branch around though, we will get to it eventually. I have to fix problems with CMake provided system PCL/Eigen problems in the meantime.

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.

None yet

4 participants