ARMmbed / mbed-cli

Arm Mbed Command Line Interface
https://os.mbed.com
Apache License 2.0
333 stars 176 forks source link

SCM and neo #70

Closed thegecko closed 8 years ago

thegecko commented 8 years ago

Why is neo strongly tied to source control management?

Having to specify hg or git up front (or any SCM) when creating a new project is mixing concerns and forces source control decisions onto users.

With the current coupling how would a user:

thegecko commented 8 years ago

Thanks @0xc0170 . I investigated further and also found that export and publish require these directories. I figured I could create a PR to ask the user to supply an SCM type prior to exporting or publishing which would fix this and #106 , but there is another reason the SCM is required initially; ignores!

Whenever an external library is added to the source, it's path is explicitly added to the ignores of the SCM, so the SCM type is also needed prior to any add.

A way to fix this is to consider flattening the .lib dependencies to exist in a single folder in the root, similar to yotta_modules. This would give us a single folder to add to the ignores[], removing the need to specify SCM up front. As an added benefit, it also means there is a single point to manage external dependencies from as well (I'm thinking integration in visual IDEs).

@mjs-arm @bogdanm @sg-

thegecko commented 8 years ago

Changing labels as I think the question is answered, but the separation of concerns is a bug.

sg- commented 8 years ago

@thegecko I like the sound of that!. Wondering how it might affect nested dependencies and if its even possible while trying to maintain backwards compatibility?

edit I read the above comment as .lib files at the top level still using nested repos. Nested and flattened deps both have inherit problems. Flattened means inclusion visibility in the context of a system is only human readable with a tool but you could catch and try to resolve duplicate dependencies. Nested means duplicate nested dependencies cause problems again either a tool fixes or use fixes. Either way, nested is more nautual on disc IMO.

screamerbg commented 8 years ago

@thegecko flattening the nested dependencies poses significant impact to the dependency handling between a library and it's sub-libraries, the integration between them, reproducibility and also workflows. To visualize this:

program
 |- libA
 `- libB
     `- libC

Flattening the above means that there is no guarantee that libB integrates with libC (and vice-versa) because the top program might reference libC at at a different revision that has never been tested/integrated with libB.

The nested dependency mechanism is to ensure that as long as every library integrates directly with it's child libraries (and child libraries with their child libraries..), there is confidence in the integration of the whole dependency tree.

Additionally, there is 1:1 mapping between the code tree as file layout on disk and the dependency tree, which is a great plus because there is no confusion between the two.

And last but not least, flattening also breaks backward compatibility with mbed Classic where all the 38000 repositories have nested dependencies, not flattened, which means that each repository takes care of it's own dependencies.

thegecko commented 8 years ago

@screamerbg , I think you misunderstand my suggestion.

mbed-os actually looks like this:

program
 |-mbed-os
    |-folderA
        |-folderB
            |-libA (via repo)
        |-folderC
            |-libB (via repo)
                |-folderD
                    |-libC (via repo)

And I'm recommending something like:

program
 |-mbed-os
    |-libraries
        |-libA (via repo)
        |-libB (via repo)
            |-libraries
                |-libC (via repo)
            |-folderD
    |-folderA
        |-folderB
        |-folderC

Which keeps the dependencies tracked by each repository, but in a single root (/libraries or similar) which can be added to the default ignores[]. It also means dependencies for each repository are managed in one place.

screamerbg commented 8 years ago

@thegecko Currently mbed CLI (neo) handles ignores very well regardless of the layout of the libraries. Also it doesn't overwrite/modify the user ignores. Just run "mbed sync" and it would take care of all sub-repo ignores.

Regarding the layout, it's unclear from the schematic above where do repos and library references (.lib) live. All in "/libraries" or separately? Also do you mean that my top app "mbed-example-app" would have folder "/libraries" with mbed-os inside instead of being in the root?

To understand your proposal better, can you translate the current (slightly stripped) mbed-os based application layout below:

mbed-example-app (repo)
|- mbed-os (repo)
|  |
|  |- core (folder)
|  |  |- mbed-rtos (folder)
|  |  |- mbedtls.lib (ref)
|  |  |- mbedtls (repo)
|  |  |- uvisor-mbed-lib (repo)
|  |  `- uvisor-mbed-lib.lib (ref)
|  |
|  |- frameworks (folder)
|  |  |- greentea-client (repo)
|  |  |- greentea-client.lib (ref)
|  |  |- unity (repo)
|  |  |- unity.lib (ref)
|  |  `- utest (folder)
|  |
|  |- net (folder)
|  |  |- coap-service (repo)
|  |  |- coap-service.lib (ref)
|  |  |- LWIPInterface (folder)
|  |  |- mbed-client (repo)
|  |  |- mbed-client.lib (ref)
|  |  |- mbed-client-c (repo)
|  |  |- mbed-client-c.lib (ref)
|  |  |- mbed-client-classic (repo)
|  |  |- mbed-client-classic.lib (ref)
|  |  |- mbed-client-mbedtls (repo)
|  |  |- mbed-client-mbedtls.lib (ref)
|  |  |- mbed-mesh-api (repo)
|  |  |- mbed-mesh-api.lib (ref)
|  |  |- mbed-trace (repo)
|  |  |- mbed-trace.lib (ref)
|  |  |- nanostack-hal-mbed-cmsis-rtos (repo)
|  |  |- nanostack-hal-mbed-cmsis-rtos.lib (ref)
|  |  |- nanostack-libservice (repo)
|  |  |- nanostack-libservice.lib (ref)
|  |  |- NetworkSocketAPI (folder)
|  |  |- sal-iface-6lowpan-morpheus-private (repo)
|  |  |- sal-iface-6lowpan-morpheus-private.lib (ref)
|  |  |- sal-stack-nanostack-private (repo)
|  |  `- sal-stack-nanostack-private.lib (ref)
|  |
|  `- tools (to become repo)
|
|- mbed-os.lib (ref)
|
`- main.cpp
thegecko commented 8 years ago

screamerbg removed the bug label an hour ago

I still believe requiring SCM is a bug.

Currently mbed CLI (neo) handles ignores very well regardless of the layout of the libraries

This needs to be mimicked in other IDEs and isn't trivial. For this to work, you need to add to an ignore file as you go, hence the need to have SCM up front. A single root of libraries makes adding /libraries to an ignore file during export/publish trivial.

All in "/libraries" or separately?

The .lib files and subsequent repos would live in /libraries

do you mean that my top app "mbed-example-app" would have folder "/libraries" with mbed-os inside instead of being in the root?

Yes

can you translate the current (slightly stripped) mbed-os based application

Sure, something like:

mbed-example-app (repo)
|
|-libraries
|  |
|  |- mbed-os (repo)
|  |  |
|  |  |-libraries
|  |  |  |
|  |  |  |- mbedtls.lib (ref)
|  |  |  |- mbedtls (repo)
|  |  |  |- greentea-client (repo)
|  |  |  |- greentea-client.lib (ref)
|  |  |  |- unity (repo)
|  |  |  |- unity.lib (ref)
|  |  |  |- mbed-client (repo)
|  |  |  |- mbed-client.lib (ref)
|  |  |  |- mbed-client-c (repo)
|  |  |  |- mbed-client-c.lib (ref)
|  |  |  |- mbed-client-classic (repo)
|  |  |  |- mbed-client-classic.lib (ref)
|  |  |  |- mbed-client-mbedtls (repo)
|  |  |  |- mbed-client-mbedtls.lib (ref)
|  |  |  |- mbed-mesh-api (repo)
|  |  |  |- mbed-mesh-api.lib (ref)
|  |  |  |- mbed-trace (repo)
|  |  |  |- mbed-trace.lib (ref)
|  |  |  |- nanostack-hal-mbed-cmsis-rtos (repo)
|  |  |  |- nanostack-hal-mbed-cmsis-rtos.lib (ref)
|  |  |  |- nanostack-libservice (repo)
|  |  |  |- nanostack-libservice.lib (ref)
|  |  |  |- sal-iface-6lowpan-morpheus-private (repo)
|  |  |  |- sal-iface-6lowpan-morpheus-private.lib (ref)
|  |  |  |- sal-stack-nanostack-private (repo)
|  |  |  |- sal-stack-nanostack-private.lib (ref)
|  |  |  |- uvisor-mbed-lib (repo)
|  |  |  |- coap-service (repo)
|  |  |  |- coap-service.lib (ref)
|  |  |  `- uvisor-mbed-lib.lib (ref)
|  |  |
|  |  |- core (folder)
|  |  |  `- mbed-rtos (folder)
|  |  |
|  |  |- frameworks (folder)
|  |  |  `- utest (folder)
|  |  |
|  |  `- net (folder)
|  |     |- LWIPInterface (folder)
|  |     `- NetworkSocketAPI (folder)
|  |
|  `- mbed-os.lib (ref)
|
|- tools (to become repo)
|
`- main.cpp

And the .lib files in each libraries folder could also be merged into a single file.

thegecko commented 8 years ago

This discussion has gone quiet for 2 weeks.There could be an easier interim solution which at least removes the reliance of SCM up front.

Assuming SCM is required only to record whether a library folder is ignored, why not determine this prior to any publish or export by adding the SCM switch to the publish or export function?

The folders to be ignored could then be determined by:

-or-

jenia81 commented 8 years ago

We are now integrating Provisioning into Morpheus and I can tell you that indeed that the coupling of mbed-cli to SCM is a problem, from user perspective. What I wanted to do is to import mbed os and run mbed compile with my sources as input. I can't do that without use mbed new or git new I think that the user should be able to import mbed os and compile his sources without the need to add his source code to the SCM

screamerbg commented 8 years ago

@jenia81 There is a fix for this in the latest PR which allows you to compile and expert source code without the root being SCM. Can you try it before its merged? PR #147

jenia81 commented 8 years ago

I tested, it works!

jenia81 commented 8 years ago

When is this going to be merged with mbed-cli?

jenia81 commented 8 years ago

@screamerbg We have another issue that is related to the coupling of mbed-cli with source control. https://github.com/ARMmbed/mbed-cli/issues/159 Are those issues related? The issue still appears with your branch

screamerbg commented 8 years ago

Can you please try with the latest version 0.5.0?

jenia81 commented 8 years ago

@screamerbg We'll try tomorrow. What's is this PR should fix? The git inside git issue?

screamerbg commented 8 years ago

@jenia81 Yes