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

Skip mbed-os import in subdirs #474

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Skip mbed-os import in subdirs #474

wants to merge 2 commits into from

Conversation

wdwalker
Copy link

The change is rather straightforward - if we come across an mbed-os.lib file in a library imported by an app, skip it. My test set up is as follows:

  • A library that has an mbed-os.lib in it.
  • An app that imports that library as well as mbed-os (e.g., has two *.lib files).

mbed import of both yields the desired results:

  • Importing the library alone gets me an mbed-os to work with
  • Importing the app gets me the library and mbed-os in the top level directory with no redundant mbed-os import

My knowledge of mbed-cli.py is limited, however, so the above may not be sufficient. Please review - I will not be at all offended by rejection.

Fixes #472 - mbed-cli can clone mbed-os.lib more than once.

@wdwalker
Copy link
Author

NOTE: this can be generalized by keeping track of all the repos imported at the cwd_root level. A solution for this could be to keep a global set that holds the names of all the *.lib files that have been used to import so far. This would still be isolated to Repo.getlibs.

Alternatively, one could use a similar global set, but go a little deeper: save the actual URLs that have been used. This would be managed in Repo.fromurl. This would be a little awkward, however, in that Repo.fromurl would return None when a redundant import is found. I assume we'd want some action() call stating why something was skipped.

@screamerbg
Copy link
Contributor

screamerbg commented Apr 13, 2017

@wdwalker firstly thanks for the PR.

The implementation as it is creates ambiguity about what version will be used in case of conflicting dependencies. As an example a library may have mbed-os.lib at rev #ABC, but the program may have mbed-os.lib at rev #XYZ. The user end will end up with mbed-os#XYZ if they import the program, which is different if they import the library (which would give them #ABC otherwise).

Also we'd like to encourage a clean development model where a library is NOT the root of the tree. Here's some reasoning behind it:

  1. A library doesn't contain the main application code, otherwise it would really be a program...
  2. A library should live side-by-side mbed-os and handle it's own dependencies and sub-libraries
  3. mbed-os is NOT a sub-library of a library (it's the OS) and lives at the root of the tree (again at program level, not library)
  4. The program defines which mbed-os revision is used, not a library (the requirement for mbed-os to be at the root of the program is not by accident).

Do you feel that a development tree like below would work for you?

example_program
 |- library-dev
 |- mbed-os
 |- main.cpp
 |- other-stuff.h
 `- other-stuff.cpp

@wdwalker
Copy link
Author

@screamerbg - many thanks for the response. I think the development tree looks fine and it is what I've been working with in my own sandbox. What I'm looking to do is have my library contain its own TESTS and such; those need mbed-os headers and sources to compile against. This will more easily enable library development independently of an app and automated CI.

I definitely agree about the ambiguity issue and also agree that the app should dictate the mbed-os version. One might argue that knowing which version of the mbed-os sources the library was developed against is useful info, though. Some sort of compatibility framework might be nice (though out of scope for this PR).

Today, I do an 'mbed add mbed-os' whenever I work with my library directly and I also do the same in the library's .travis.yml. I was hoping to be able to avoid this via the patch proposed in this PR, but I'm OK with continuing to do what I do today.

Happy to close this PR if you think it's a dead end. Please let me know.

@screamerbg
Copy link
Contributor

@wdwalker I think we should rather show a good visible warning when duplicate clones are detected. This will help the user understand that there might be underlying problem and duplicate code.

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.

2 participants