per profile build dirs

[bmcdonnell-ionx]

Summary

Attempt to address #615.

When a build dir is not specified, mbed compile creates a new per-build-profile subdirectory. This prevents full rebuilds when switching build profiles.

Command	Build Dir
mbed compile	BUILD/<target>/<toolchain>/DEFAULT/
mbed compile --profile debug	BUILD/<target>/<toolchain>/DEBUG/
mbed compile --profile=myprofiles/superoptimized-cpp11.json	BUILD/<target>/<toolchain>/SUPEROPTIMIZED-CPP11/
mbed compile --library --profile debug	BUILD/libraries/<project-name>/<target>/<toolchain>/DEBUG/

Notes

1. My Python knowledge is limited, so there may be better ways to do things, or more error handling required, etc.
2. I'd be happy with lower case, but followed what seems to be the convention (upper case) here. Feel free to change it.

Questions

1. Are there any programs or scripts that consume builds, which would need their code or paths updated?
2. Other concerns?

[theotherjimmy]

@bmcdonnell-ionx using default as the "default profile" is correct, and may cause unneeded rebuilds.

[theotherjimmy]

Please avoid functions within functions. This makes it much more difficult to track what state is used by/shared with this function.

[bmcdonnell-ionx]

I nested it because it's a helper function with limited scope, and likely not useful elsewhere.

Is your issue the fact that it's using a variable (profile) from its outer scope? Would adding that as a parameter be sufficient?

Or is there some other issue I'm missing? Or is it just a rule that you want to be strictly followed, because of the potential for abuse, i.e. using variables from outer scope?

[theotherjimmy]

Yes, my preference is to avoid using closures. Not because of their potential for abuse, but because it increases the amount of code I have to read to understand what state affects a single function. A function on it's own can change behavior based on global variable and inputs. A closure, a function within the body of another function, adds that function's scope and variable into the pool of variables that may impact it's state.

Helper functions may be prefixed with an _ to mark them private, by python convention.

TL;DR: I find self-contained functions easier to read and understand.

[theotherjimmy]

It also might be useful to make the same change to the mbed test sub-command.

[theotherjimmy]

Further, be aware that this will affect linker script command line length, but that's not a limitation at this time.

[bmcdonnell-ionx]

@theotherjimmy,

using default as the "default profile" is correct, and may cause unneeded rebuilds.

I don't follow. Please clarify/elaborate.

[theotherjimmy]

the default profile is always "develop". The use of "DEFAULT" for the default profile's name will force the user to recompile mbed-os when they explicitly specify the develop profile.

[screamerbg]

@bmcdonnell-ionx Firstly, thanks for the contribution. Unfortunately in it's current state, this will break any CI or script that someone made which expects that the binary/output files for the default profile to be the BUILD/<target>/<toolchain>/ folder, e.g. the location of BUILD/<target>/<toolchain>/program.bin.

Therefore my proposal to workaround this and keep it backwards compatible, is to change the pattern to BUILD/<target>/<toolchain>_<profile>/ and if profile is specified and keep it to BUILD/<target>/<toolchain>/ for the default/non-specified profile.

[bmcdonnell-ionx]

@theotherjimmy,

the default profile is always "develop".

Yes, I thought I had read that somewhere. But I couldn't figure out how that was happening in the code.

The use of "DEFAULT" for the default profile's name will force the user to recompile mbed-os when they explicitly specify the develop profile.

Admittedly, yes.

I didn't want to hard code append_str = 'DEVELOP', and potentially have that break later. Is there a way in the function where I made the changes to determine what the default profile is?

[bmcdonnell-ionx]

@screamerbg, I don't understand how your proposal doesn't have the same issue that you described as mine does, unless the testing tools only use the default build profile. Is that the case?

[screamerbg]

@bmcdonnell-ionx Correct. The testing tools only use the default build profile.

[bmcdonnell-ionx]

@screamerbg, shall I implement your suggestion then, or wait for more feedback?

[theotherjimmy]

I agree with @screamerbg's suggestion of build paths. I think that's a good path forward.

[theotherjimmy]

Is there a way in the function where I made the changes to determine what the default profile is?

Not that I'm aware of.

[bmcdonnell-ionx]

@screamerbg, et al,

my proposal to workaround this and keep it backwards compatible, is to change the pattern to BUILD/<target>/<toolchain>_<profile>/ and if profile is specified and keep it to BUILD/<target>/<toolchain>/ for the default/non-specified profile.

Some toolchains already have underscores in their names, e.g. GCC_ARM. Should I add one underscore or two for the append? e.g.

BUILD/LPC4088/GCC_ARM_DEVELOP/
or
BUILD/LPC4088/GCC_ARM__DEVELOP/

I weakly favor two.

[theotherjimmy]

Unpopular opinion of the day: Use - instead!

[theotherjimmy]

2 _ should be fine.

[bmcdonnell-ionx]

OK, I've made the requested changes.

This is working fine for me on project builds, and I tried it a couple times on library builds. I don't know how to test it on the mbed test subcommand, so I leave that to you.

Is the CI py3 test failure my fault?

[theotherjimmy]

@bmcdonnell-ionx It looks like the py3 CI failure may be your fault:

[ERROR] cannot use a string pattern on a bytes-like object

[bmcdonnell-ionx]

How can I reproduce this error on my local system, or at least get a line number to know where to look?

I tried installing Python 3.7.0 (notably not 3.6, which the CI is using), and in mbed.py changing #!/usr/bin/env python to #!/c/Path/to/my/Python/Python37/python for testing, but mbed compile still worked normally. I also then deleted my main.cpp and ran mbed compile --source=. --source=mbed-os/TESTS/integration/basic -j 0 (the CI command which failed), but it worked fine, too.

[theotherjimmy]

@bmcdonnell-ionx You may have to install it with pip3, as that's how the mbed executable is created.

[theotherjimmy]

you could also try python3 -m mbed instead of mbed.

[bmcdonnell-ionx]

You may have to install it with pip3, as that's how the mbed executable is created.

you could also try python3 -m mbed instead of mbed.

How would I install from source with pip?

(Note, Python3 is not in my path.)

I tried /c/Path/to/my/Python/Python37/python setup.py install, which appears to succeed. But then in a test project, I get this.

$ /c/Path/to/my/Python/Python37/python -m mbed compile
Traceback (most recent call last):
  File "C:\Path\to\my\Python\Python37\lib\runpy.py", line 193, in _run_module_as_main
    "__main__", mod_spec)
  File "C:\Path\to\my\Python\Python37\lib\runpy.py", line 85, in _run_code
    exec(code, run_globals)
  File "C:\Path\to\my\Python\Python37\lib\site-packages\mbed_cli-1.7.3-py3.7.egg\mbed\__main__.py", line 1, in <module>
ImportError: cannot import name 'main' from 'mbed' (C:\Path\to\my\Python\Python37\lib\site-packages\mbed_cli-1.7.3-py3.7.egg\mbed\__init__.py)

[theotherjimmy]

That's odd.

[theotherjimmy]

pip install -U . from the source directory will install from source.

[bmcdonnell-ionx]

@theotherjimmy,

It looks like the py3 CI failure may be your fault:

[ERROR] cannot use a string pattern on a bytes-like object

Actually, I don't think it is, because I get the same error when using mbed-cli master.

Install Mbed CLI master branch in Python3 from source:

me@pc MINGW64 /c/dev_temp/mbed-cli (master)
$ git log -n1 --oneline
667814f (HEAD -> master, origin/master, origin/HEAD, github-ionx/master) Merge pull request #704 from daid/patch-3

me@pc MINGW64 /c/dev_temp/mbed-cli (master)
$ pip3 install -U .
Processing c:\dev_temp\mbed-cli
Installing collected packages: mbed-cli
  Found existing installation: mbed-cli 1.7.3
    Uninstalling mbed-cli-1.7.3:
      Successfully uninstalled mbed-cli-1.7.3
  Running setup.py install for mbed-cli: started
    Running setup.py install for mbed-cli: finished with status 'done'
Successfully installed mbed-cli-1.7.3

Run the failing test:

me@pc MINGW64 /c/dev_temp/mbed-cli (master)
$ cd ../test-build-paths/

me@pc MINGW64 /c/dev_temp/test-build-paths (master)
$ cd mbed-os; git log -n1 --oneline; cd ..
62f8b922b (HEAD -> mbed-os-5.9, tag: mbed-os-5.9.2, origin/mbed-os-5.9) Merge pull request #7375 from ARMmbed/release-candidate

me@pc MINGW64 /c/dev_temp/test-build-paths (master)
$ /c/path/to/Python/Python37/Scripts/mbed compile --source=. --source=mbed-os/TESTS/integration/basic -j 0
[ERROR] cannot use a string pattern on a bytes-like object
[mbed] WARNING: If you're using Python 3 with Mbed OS 5.8 and earlier versions, Python errors will occur when compiling, testing and exporting
---
[mbed] ERROR: "c:\path\to\python\python37\python.exe" returned error.
       Code: 1
       Path: "C:\dev_temp\test-build-paths"
       Command: "c:\path\to\python\python37\python.exe -u C:\dev_temp\test-build-paths\mbed-os\tools\make.py -t GCC_ARM -m LPC4088 --source . --source mbed-os/TESTS/integration/basic --build .\BUILD\LPC4088\GCC_ARM -j 0"
       Tip: You could retry the last command with "-v" flag for verbose output
---

[theotherjimmy]

@bmcdonnell-ionx Touche

[theotherjimmy]

@cmonr How did this get onto master?

[cmonr]

@theotherjimmy No idea. Will look at root causing today.

[theotherjimmy]

@screamerbg

Also what's wrong with a single underscore _ as a separator?

Nothing is wrong with it, except that there is the potential for it to be ambiguous:
GCC_ARM_DEBUG could be either GCC_ARM with a DEBUG profile or GCC with an ARM_DEBUG profile.

---

I thought that - would be more unpopular. I should have pushed for it harder then. All well.

[bmcdonnell-ionx]

@screamerbg, @theotherjimmy,

As @theotherjimmy pointed out, using something different from a single underscore separator (e.g. __, -, or other) resolves an ambiguity here.

I think using - as a separator in this context looks ugly and weird due to mixing asymmetrically with the underscores, e.g. GCC_ARM-DEBUG. Also of course here the compiler GCC_ARM is separated from the build config DEBUG; but because the - is shorter than the _, IMO that implies that GCC is the compiler, and ARM-DEBUG the build config.

Personally, I don't find a double underscore __ confusing. I thought it was a common enough usage. (3 would throw me off, though.)

That said, if your team reaches consensus here on the - or something else, I'll change it.

[theotherjimmy]

@bmcdonnell-ionx @screamerbg .?

[bmcdonnell-ionx]

@theotherjimmy,

Here is the . example: GCC_ARM.DEBUG

I'm not a fan of . in directory names, as IME it's usually only used in file names. I think it may introduce more confusion than it resolves.

Also it has the "shorter" issue (mistaking it to mean GCC compiler, ARM.DEBUG config).

That said, again, I will defer to consensus.

[theotherjimmy]

I'm out of symbol keys on my keyboard that normally work as separators (and are not ).

[theotherjimmy]

OR I'm not : or ;?

[theotherjimmy]

GCC_ARM:DEBUG or if you want to go very C++: GCC_ARM::DEBUG?

[bmcdonnell-ionx]

Yeah, those ( : ;) would definitely be bad.

[theotherjimmy]

While we're at it: I think that # ` $ ~ & = * + ! | and any braces would be bad too.

[screamerbg]

Nothing is wrong with it, except that there is the potential for it to be ambiguous:
GCC_ARM_DEBUG could be either GCC_ARM with a DEBUG profile or GCC with an ARM_DEBUG profile.

I thought that - would be more unpopular. I should have pushed for it harder then. All well.

One underscore is ambiguous, and so is 2 but may be less, shall we make them 3? :)
And no, double underscore is not common usage. Looks more like typo than anything (was holding Shift+_ longer that I should have). Or some sort of non-Latin characters converter where "Massive Attack'1998 - (01) Angel.mp3" becomes "Massive_Attack_1998_-__01__Angel.mp3"

Are we really thinking that someone will use GCC_DEBUG for ARM Compiler?? Let's not overcomplicate this and use either single _, or -.

[bmcdonnell-ionx]

double underscore is not common usage.

One example that comes to mind is library preprocessor macros.

https://stackoverflow.com/a/224420
https://gcc.gnu.org/onlinedocs/cpp/Standard-Predefined-Macros.html

Apparently they're used in Python, too.

https://stackoverflow.com/a/1301369

Are we really thinking that someone will use both GCC_DEBUG for ARM Compiler??

Whether or not it's used, it's still ambiguous to read.

(I still will defer to consensus - that just doesn't appear to have occurred yet.)

[screamerbg]

@bmcdonnell-ionx

One example that comes to mind is library preprocessor macros.

https://stackoverflow.com/a/224420
https://gcc.gnu.org/onlinedocs/cpp/Standard-Predefined-Macros.html

Apparently they're used in Python, too.

https://stackoverflow.com/a/1301369

Not sure how this is relevant. Can you elaborate?

[bmcdonnell-ionx]

@screamerbg, you said double underscores are not common usage. It may not be "common" per se, but I had said that that I thought it was a "common enough" usage.

Those are examples of double underscores used by programmers. Mbed OS users are programmers - and using C/C++, so I figure they're likely to have seen double underscores used at least in preprocessor macros.

[screamerbg]

@bmcdonnell-ionx Not sure what's the point you're trying to make. Programmers would mix preprocessing macros with output build folders??

[bmcdonnell-ionx]

I'm just saying that double underscore is a thing that most Mbed OS users have probably seen, and so should not be confused by, IMO, contrary to your initial statement. (I don't understand the "confusion", either.)

[screamerbg]

This is about output folders naming, not code variables. I don't see the relevance.

Also following your argument, developers are familiar with dot in coding as well. Why not GCC_ARM.DEBUG then?

[theotherjimmy]

@screamerbg The important part is that python uses __, or the "dunderscore", for various file-system related things. I think that - . : are better than __ though.

[theotherjimmy]

Aside, as for bike shedding we could not have made a better example if we tried.

[theotherjimmy]

Code looks great, BTW.

[bmcdonnell-ionx]

@screamerbg, the relevance is I didn't think anyone was going to say "what's that thing?" about the double underscore. You did?

A weak recommendation against . in dir names:
https://askubuntu.com/a/417446

More characters that are discouraged are: space and dot . (source)

• Command line tools are harder to use when you have spaces in names (harder, not impossible)
• Dots are used in RegEx (e.g. when you want to use grep). A leading dot makes a file hidden by convention in Linux. In Windows, dots are used in the file extension, which is used for file type detection.

[bmcdonnell-ionx]

@theotherjimmy,

I think that - . : are better than __ though.

: is not allowed in Windows file/dir names.

https://docs.microsoft.com/en-us/windows/desktop/FileIO/naming-a-file

[theotherjimmy]

Yep, so - it is? We have tests that already have that odd naming convention e.g. TESTS/mbed_hal/flash => tests-mbed_hal-flash

[bmcdonnell-ionx]

Good example.

so - it is?

If @screamerbg still wants that, then yes.

[bmcdonnell-ionx]

I frequently use - as a word delimiter within filenames. I just noticed that I have some profiles in my project that use -, such as:

• build-profile-cpp11-debug.json
• build-profile-cpp11-develop.json
• build-profile-cpp11-release.json

With - as the delimiter here, I get e.g. BUILD/LPC4088/GCC_ARM-BUILD-PROFILE-CPP11-RELEASE/. So personally, I still prefer __ (double underscore).

That said, I take @screamerbg's 👍 on the previous 2 comments to mean he favors -, so I pushed that change. If that remains your preference, you can merge after CI passes.

[screamerbg]

Looks great! Thank you!

[screamerbg]

LGTM. Merge at will @theotherjimmy

[theotherjimmy]

I will, when testing finishes.