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

move drake-distro/drake up one level; rename drake-distro -> drake in all docs. #6996

Closed
RussTedrake opened this issue Sep 10, 2017 · 20 comments

Comments

@RussTedrake
Copy link
Contributor

as we purge all of the externals-handling via cmake (and therefore loose the drake-distro/externals directory), I think it would be appropriate to make that change? Bazel's handling of externals feels less like a superbuild than the cmake externals version did.

If yes, it would be a big change but the time is probably now (before we post our first monthly release).

Is it worth it?

@naveenoid
Copy link
Contributor

++1!

@sherm1
Copy link
Member

sherm1 commented Sep 11, 2017

That would be so nice!!

@jwnimmer-tri
Copy link
Collaborator

I'm not sure why we should rush it before a monthly release? The installed binary copy of Drake would change relatively little (I think only the resource searching would change); downstream include paths like #include <drake/systems/framework/system.h> would be unchanged.

As to the proposal itself, I would support changing e.g. drake-distro/drake/common in a source checkout to lose the duplication of the drake word, but I'm not sure why drake/src/comon is better? Why not just move drake-distro/drake/* up one level, so in git we have drake/common and drake/doc and drake/setup and etc?

@RussTedrake
Copy link
Contributor Author

i think of the monthly release as being both a binary release and a tag for people to build from source.

i don't have a strong preference about moving it up a level or not. i thought it would be more standard to separate the src from the e.g. build/ bazel-bin/ etc with an explicit src subdirectory.

@jamiesnape
Copy link
Contributor

jamiesnape commented Sep 13, 2017

I agree with @jwnimmer-tri that we should move everything up one level, though I think I agree with @RussTedrake that it would be nice to do so before we tag a release.

(if we are doing filesystem housekeeping, I would also like the matlab under the bindings directory)

@EricCousineau-TRI
Copy link
Contributor

EricCousineau-TRI commented Sep 13, 2017

+1 for moving it up one level.

I think that if we use src/, it may be a tad odd to use it externally via Bazel, such as referencing @drake//src/manipulation:robot_plan_interpolator.
(Then again, with how tensorflow is structured, it looks like you'd have to do @tensorflow//tensorflow:core, like what we presently have with something like @drake//drake:common.)

Potential Bike-Shedding:
Minor question, but just to check, would we simply change the resource locations as well (e.g. FindResource("manipulation/models/...") rather than FindResource("drake/manipulation/models/...")), such that (a) FindResource does not have to do special prefix-stripping operations and (b) is not dependent on the repository's name?

Also, given that the paths themselves don't involve drake/..., should we lock drake::FindResource down to be drake-specific (which is what the documentation presently implies) and used only for drake examples / testing, possibly discouraging its use externally (unless these are drake-centric externals)?

\cc @stonier regarding the role of FindResource in drake for installation, external usage, etc.

@stonier
Copy link
Contributor

stonier commented Sep 13, 2017

+1 for moving it up a level too and +1 before a release.

I expect there will be a few changes around the installation and FindResource. Right now the installed resources sit inside share/drake/drake/... which in itself is a little odd.

@RussTedrake RussTedrake changed the title rename drake-distro/drake to drake/src? move drake-distro/drake up one level; rename drake-distro -> drake in all docs. Sep 30, 2017
@RussTedrake
Copy link
Contributor Author

drake-distro/externals are gone. Thanks @jamiesnape !
i think it's time to schedule this (or decide it's not worth it). it probably needs a flag day, and some accompanying instructions for folks that will have a potentially confusing merge from their dev branches.

@jwnimmer-tri
Copy link
Collaborator

I am going to examine how to go about this.

@jamiesnape
Copy link
Contributor

I think it would be best to delay until I have stripped the rest of the CMake, but no harm in planning.

@jwnimmer-tri
Copy link
Collaborator

Right. I'm preparing patchsets that assume most or all CMakeLists.txt are gone (and so the patchsets don't touch the CMake code, and wouldn't pass CI right now), and I'll wait to PR them until other work lands first (CMake removal, Trusty removal, etc.) -- though a few tidy-ups might PR before then.

@jwnimmer-tri
Copy link
Collaborator

The main rename has been merged. If you have rebase or merge trouble, the first suggestion is:

  • First, rebase or merge up to 7f23076. This is the last revision prior to the file rename.
  • Second, rebase or merge up to c84bceb. This is the first revision that contains the rename.

The first merge should go as usual -- any merge conflicts should be as expected with normal workflow, and resolved as normal. The second merge is just renames, so should do git magic and occur without trouble.

The only remaining problems was if your feature branch contained files that were not yet on master. You will probably have to git mv these into their new home manually (removing "drake" from their top-level folder name).

@RussTedrake
Copy link
Contributor Author

N.B. - the installation instructions on sphinx (http://drake.mit.edu/from_source.html) still refer to drake-distro, but should now just be drake. hoorah!

@jwnimmer-tri
Copy link
Collaborator

So far it looks like we might be able to survive with the new layout. Next up, I'll remove some of the transition shims left over on master, as well as som documentation updates.

@jwnimmer-tri
Copy link
Collaborator

Most code and docs are revised now, so I'm lowering the priority. There are still BUILD file cleanups pending.

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

No branches or pull requests

8 participants