Deprecate compile --build-properties flag in favor of new featureful --build-property flag #1044

silvanocerza · 2020-10-22T15:26:52Z

Please check if the PR fulfills these requirements

The PR has no duplicates (please search among the Pull Requests
before creating one)
The PR follows
our contributing guidelines
Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)

What kind of change does this PR introduce?

Deprecate --build-properties flag in compile command and add a new --build-property flag

What is the current behavior?

The following commands would all fail because of quotes (") in them.

export COMPILE_OPTIONS='-DMY_DEFINE="hello world"'
arduino-cli compile -b arduino:avr:uno --build-properties "build.extra_flags='$COMPILE_OPTIONS'" ./my-sketch

arduino-cli compile -b arduino:avr:uno --build-properties="build.extra_flags=\"-DMY_DEFINE=\"hello world\"\"" ./my-sketch

arduino-cli compile -b arduino:avr:uno --build-properties="build.extra_flags=\"-DMY_DEFINE=\"hello world\"\"" ./my-sketch

arduino-cli compile -b arduino:avr:uno --build-properties='compiler.cpp.extra_flags="-DPIN=2" "-DSSID=\"This is a String\"' ./my-sketch

What is the new behavior?

The --build-property is introduced to substitute in the future the --build-properties flag, it supports the use of quotes but has a different syntax.
Different build flags can't be specified using the same --build-property flag, nor you can specify the same build flag on separate --build-property otherwise they'll get overridden.

These are some examples.

  ./arduino-cli compile -b arduino:avr:uno /home/user/Arduino/MySketch
  ./arduino-cli compile -b arduino:avr:uno --build-property="build.extra_flags=\"-DMY_DEFINE=\"hello world\"\"" /home/user/Arduino/MySketch
  ./arduino-cli compile -b arduino:avr:uno --build-property="build.extra_flags=-DPIN=2 \"-DMY_DEFINE=\"hello world\"\"" /home/user/Arduino/MySketch
  ./arduino-cli compile -b arduino:avr:uno --build-property=build.extra_flags=-DPIN=2 --build-property="compiler.cpp.extra_flags=\"-DMY_DEFINE=\"hello world\"\"" /home/user/Arduino/MySketch

Does this PR introduce a breaking change?

No.

Other information:
Fixes Can't include quoted strings in --build-properties args #1037, Build flags set in my boards.txt file not used in the compilation command #210 and probably also using multiple linking flags when overriding a build property #532 and Provide dedicated sets of "extra flags" properties for platform developers and users #846.

See how to contribute

cmaglie · 2020-10-22T15:46:50Z

legacy/builder/types/context.go

-	ctx.CustomBuildProperties = strings.Split(opts.Get("customBuildProperties"), ",")
-	ctx.OptimizationFlags = opts.Get("compiler.optimization_flags")
-}
-


Why did you remove this one? Is this function unused?

Yep, completely unused. I was digging around trying to understand how to fix the issue and stumbled upon it, noticed it wasn't used and decided to delete since I was there.

I checked, it's used in the arduino-builder, so we need it: https://github.com/arduino/arduino-builder/blob/642e2cd708dd024e658067d383387691f7e79abd/main.go#L244

cmaglie · 2020-10-22T15:56:56Z

cli/compile/compile.go

-	command.Flags().StringSliceVar(&buildProperties, "build-properties", []string{},
-		"List of custom build properties separated by commas. Or can be used multiple times for multiple properties.")
+	command.Flags().StringArrayVar(&buildProperties, "build-properties", []string{},
+		"List of custom build properties separated by spaces. Or can be used multiple times for multiple properties.")


Since if you want to change multiple properties you now need to repeat the flag multiple times I think that the correct description is something along the line of:

Suggested change

"List of custom build properties separated by spaces. Or can be used multiple times for multiple properties.")

"Override a build property with a custom value. Can be used multiple times for multiple properties.")

silvanocerza · 2020-10-27T08:52:23Z

I've updated the PR with some changes, I marked the --build-properties flag as deprecated in favor of a new --build-property flag that works a bit differently but supports quotes in defines and similar things.

ubidefeo · 2020-10-27T15:53:51Z

This took a while to test, had to look hard for a way to break it but haven't found it yet.
it works as far as I'm concerned

silvanocerza · 2020-10-27T16:30:38Z

This took a while to test, had to look hard for a way to break it but haven't found it yet.
it works as far as I'm concerned

This is a first people! Marking it on the calendar! 📆

I'll be waiting for a review by @cmaglie anyway before merging. 😄

silvanocerza · 2020-10-27T16:33:15Z

Also would love some feedback from @obra, @bxparks and @artynet.
You can find a testable build on here: https://github.com/arduino/arduino-cli/pull/1044/checks?check_run_id=1310376266
There's an Artifacts dropdown menu on the top right with download links for different platforms.

obra · 2020-10-27T16:45:02Z

I’ll do my best to give it a workout today. Does arduino-cli have a written policy about what “deprecation” means?

Some projects, for example, have a policy of deprecations warning for two releases before failing with a message that the facility was removed for another n releases. Some projects explicitly state that every .0 is fair game for breakage.

silvanocerza · 2020-10-27T16:48:34Z

I’ll do my best to give it a workout today. Does arduino-cli have a written policy about what “deprecation” means?

Thanks, appreciate it. :)

Some projects, for example, have a policy of deprecations warning for two releases before failing with a message that the facility was removed for another n releases. Some projects explicitly state that every .0 is fair game for breakage.

We're working on it, we're exploring different alternatives.

obra · 2020-10-27T16:57:16Z

Some projects, for example, have a policy of deprecations warning for two releases before failing with a message that the facility was removed for another n releases. Some projects explicitly state that every .0 is fair game for breakage.

We're working on it, we're exploring different alternatives.

I can respect that answer :) My background comes from managing Perl's backwards-compatibility policy, which is, roughly "do not break on a user unless it is unavoidable." Being that focused on not hurting users comes with a huge maintenance burden that can calcify a project if such a policy is applied to early.

Is there a place that it would be useful for me to comment about what I as a consumer of the tool (and as someone who's been responsible for writing such policies) might want? It's 100% ok if the answer is "feedback is not useful right now"

rsora · 2020-10-27T17:38:26Z

@obra

I can respect that answer :) My background comes from managing Perl's backwards-compatibility policy, which is, roughly "do not break on a user unless it is unavoidable." Being that focused on not hurting users comes with a huge maintenance burden that can calcify a project if such a policy is applied to early.

Exactly this :) Being the CLI still in alpha and in very heavy development, we are running fast to stabilize the APIs and implement all the features we think are needed. We are trying to avoid to enforce a structured Deprecation policy ATM to not slow down the development.

However we recognize that the project gained traction, and we have users that rely on the CLI in their projects. We don't want to let our users down :) so I started to modify slightly our contribution guidelines here #1039 and our internal release documents in order to start managing the project breaking changes in the easiest way possible.

It would be great to get your feedback on the PR 👍

rsora · 2020-10-27T17:58:39Z

Let me clarify that having a structured deprecation policy is a milestone in our roadmap for the 1.0.0 stable release, so it is something we are working on.

Maybe my previous message was a bit misleading on this specific point. 😄

alranel · 2020-10-27T19:15:15Z

@obra, I'm so happy to see you again in this repository :)
Your feedback is very welcome!

obra · 2020-10-28T08:08:22Z

@rsora Is the internal release document something that can't be made public?

Poking at 0.13.. there's nowhere I see in the help or the online docs that arduino-cli is an alpha release. Ditto https://arduino.github.io/arduino-cli/latest

Now, most advanced users can probably figure out that 0.13 isn't a "release product" version number. But it looks like older alphas had 'alpha' in the filename or version.

In terms of something "lightweight" for deprecation policy before 1.0, maybe something like this would be enough?

"arduino-cli is a powerful tool that many people depend on, but it is still pre-release software. We are still hard at work making sure that arduino-cli 1.0 will be a tool that you can trust long into the future. Because of this, we may make backwards-incompatible changes at any time, but promise to do so only after careful deliberation."

cmaglie

Added a note to check the examples on the help

For the rest LGTM

cmaglie · 2020-10-28T13:40:50Z

cli/compile/compile.go

+			"  " + os.Args[0] + " compile -b arduino:avr:uno /home/user/Arduino/MySketch\n" +
+			"  " + os.Args[0] + " compile -b arduino:avr:uno --build-property=\"build.extra_flags=\\\"-DMY_DEFINE=\\\"hello world\\\"\\\"\" /home/user/Arduino/MySketch\n" +
+			"  " + os.Args[0] + " compile -b arduino:avr:uno --build-property=\"build.extra_flags=-DPIN=2 \\\"-DMY_DEFINE=\\\"hello world\\\"\\\"\" /home/user/Arduino/MySketch\n" +
+			"  " + os.Args[0] + " compile -b arduino:avr:uno --build-property=build.extra_flags=-DPIN=2 --build-property=\"compiler.cpp.extra_flags=\\\"-DSSID=\\\"hello world\\\"\\\"\"-DMY_DEFINE=\"hello world\"' /home/user/Arduino/MySketch\n",


are you sure that this last example is correct? on the help is displayed as:

./arduino-cli compile -b arduino:avr:uno --build-property=build.extra_flags=-DPIN=2 --build-property="compiler.cpp.extra_flags=\"-DSSID=\"hello world\"\""-DMY_DEFINE="hello world"' /home/user/Arduino/MySketch

and it doesn't look right to me...
To simplify your life you may use the golang verbatim string operator (backtick) so the help strings will not require triple quoting:

`--build-property="build.extra_flags=\"-DMY_DEFINE=\"hello world\"\"" /home/user/Arduino/MySketch`

another small improvement may be to drop the (optional) = sign after --build-property so instead of:

--build-property="build.extra_flags=\"-DMY_DEFINE=\"hello world\"\""

we may write

--build-property "build.extra_flags=\"-DMY_DEFINE=\"hello world\"\""

that is slightly clearer IMHO.

Right! The last example is plainly wrong, I'll fix it.

Somehow I always forget about the backtick. 🤦

bxparks · 2020-10-28T17:02:55Z

Hello, thank you for working on this issue. This is the main blocker that prevents me from using the ArduinoCLI.

Is there a reference or documentation that explains the tokenization and parsing rules for the expression on the right side of the compiler.cpp.extra_flags=? I would have assumed that the entire string to the right of the = would be passed without any processing to the compiler binary. But after some trial and error, there seems something is doing some processing of double-quotes and white spaces, but I cannot deduce the rules even after many attempts of trial and error.

The original 4th example in the compile --help output contained 2 macros, SSID and MY_DEFINE. There were at least 2 problems with it. One, an unmatched single-quote, which you fixed. Two, a missing space before the second -D. If I attempt to get the original example to work, the following does not work (I'm using the bash(1) shell where single-quoted strings are passed unprocessed to the arduino-cli binary):

$ arduino-cli compile --warnings all --verbose -b arduino:avr:uno \
    --build-property 'compiler.cpp.extra_flags=-D SSID="hello world" -D MY_DEFINE="hello world"'
[...]
.../avr-g++ [...] -D "SSID=\"hello" "world\"" -D "MY_DEFINE=\"hello" "world\"" [...]
avr-g++: error: world": No such file or directory
avr-g++: error: world": No such file or directory

It looks like the the parser is tokenizing the whitespace within the quoted "hello world". Because if we remove the white space within the quoted string, the parsing is correct:

$ arduino-cli compile --warnings all --verbose -b arduino:avr:uno \
    --build-property 'compiler.cpp.extra_flags=-D SSID="helloworld" -D MY_DEFINE="helloworld"'
[...]
.../avr-g++ [...] -D "SSID=\"helloworld\"" -D "MY_DEFINE=\"helloworld\"" [...]

The only way I have found to get around this tokenization problem is the use of unescaped, nested, double-quote (which should not work, but it does):

$ arduino-cli compile --warnings all --verbose -b arduino:avr:uno \
    --build-property 'compiler.cpp.extra_flags="-D SSID="hello world"" "-D MY_DEFINE="hello world""'
[...]
.../avr-g++ [...] "-D SSID=\"hello world\"" "-D MY_DEFINE=\"hello world\"" [...]

If I attempt to double-quote the entire string on the right side of the compiler.cpp.extra_flags=, the parsing fails:

$ arduino-cli compile --warnings all --verbose -b arduino:avr:uno \
    --build-property 'compiler.cpp.extra_flags="-D SSID="hello world" -D MY_DEFINE="hello world""'
[...]
.../avr-g++ [...] "-D SSID=\"hello world" -D "MY_DEFINE=\"hello" "world\"\"" [...]
avr-g++: error: world"": No such file or directory

It seems like nested-double-quoting only works around a single -D flag.

However, the nested double-quote escape breaks down when I try to embed a naked double-quote character inside the SSID, because the parser does not seem to know to skip over the backslashed escaped double-quote. Let's say I want the SSID to be the raw string hello" world. The following does not work:

$ arduino-cli compile --warnings all --verbose -b arduino:avr:uno \
    --build-property 'compiler.cpp.extra_flags="-D SSID="hello\" world"" "-D MY_DEFINE="hello\" world""'
[...]
.../avr-g++ [...] "-D SSID=\"hello\\" "world\"\"" "-D MY_DEFINE=\"hello\\" "world\"\"" [...]
avr-g++: error: world"": No such file or directory
avr-g++: error: world"": No such file or directory

Remarks:

The --build-property flag is definitely better than the previous --build-properties flag. But it seems like the tokenizing and parsing of the -D flags is still not fully accurate. In particular, it does not seem to support string macros with embedded spaces or embedded double-quotes. I think a lot of these problems are caused by multiple levels of tokenizing, and double-quote parsing. If you can clarify and document what those parsing rules are, I think that would help a lot.

I am still of the opinion that exposing the -D flag directly from the arduino-cli go binary is the best way to implement this feature. This would remove a number of confusing layers of parsing, tokenizing and quote and backslash escaping.

Another comment: Some of the compile --help examples show the build.extra_flags= property being overridden. But isn't the build.extra_flags property a dangerous one to override? All of the various 3rd party cores that I've looked at define a non-empty value for the build.extra_flags= property. If the end-user overrode this, it is not clear that things would work properly. The compiler.cpp.extra_flags= seems to be the property that is unused by various 3rd party cores, so can be safely overridden by end-users.

Edit: Fix typos and grammar.

bxparks · 2020-10-28T17:06:41Z

By the way, I used the following Test.ino file to test my arduino-cli commands above:

const char STRING[3] = SSID; // will trigger a compiler warning if SSID is > 2 characters

#ifdef MY_DEFINE
  #warning MY_DEFINE
#endif

void setup() { }

void loop() { }

silvanocerza · 2020-10-30T17:25:51Z

Hello, thank you for working on this issue. This is the main blocker that prevents me from using the ArduinoCLI.

Glad to help. :)

Is there a reference or documentation that explains the tokenization and parsing rules for the expression on the right side of the compiler.cpp.extra_flags=? I would have assumed that the entire string to the right of the = would be passed without any processing to the compiler binary. But after some trial and error, there seems something is doing some processing of double-quotes and white spaces, but I cannot deduce the rules even after many attempts of trial and error.

There's no documentation right now about it, we'll see to add something. The tokenization is actually done in two places, first by the cobra that parses and validates the arguments, then the legacy code of CLI collects all the flags necessary for compilation, linking, etc. and manipulates them again. The second step is kinda obscure to me for now so I'd have to investigate a bit to give you a full answer.

The original 4th example in the compile --help output contained 2 macros, SSID and MY_DEFINE. There were at least 2 problems with it. One, an unmatched single-quote, which you fixed. Two, a missing space before the second -D. If I attempt to get the original example to work, the following does not work (I'm using the bash(1) shell where single-quoted strings are passed unprocessed to the arduino-cli binary):
$ arduino-cli compile --warnings all --verbose -b arduino:avr:uno \
    --build-property 'compiler.cpp.extra_flags=-D SSID="hello world" -D MY_DEFINE="hello world"'
[...]
.../avr-g++ [...] -D "SSID=\"hello" "world\"" -D "MY_DEFINE=\"hello" "world\"" [...]
avr-g++: error: world": No such file or directory
avr-g++: error: world": No such file or directory

That was actually a mistake by my side, there was an extra define that should have not been there, the current 4th example is this one:

compile -b arduino:avr:uno --build-property build.extra_flags=-DPIN=2 --build-property "compiler.cpp.extra_flags=\"-DSSID=\"hello world\"\"" /home/user/Arduino/MySketch

It looks like the the parser is tokenizing the whitespace within the quoted "hello world". Because if we remove the white space within the quoted string, the parsing is correct:
$ arduino-cli compile --warnings all --verbose -b arduino:avr:uno \
    --build-property 'compiler.cpp.extra_flags=-D SSID="helloworld" -D MY_DEFINE="helloworld"'
[...]
.../avr-g++ [...] -D "SSID=\"helloworld\"" -D "MY_DEFINE=\"helloworld\"" [...]

This can be avoided by wrapping each define in quotes like this:

--build-property 'compiler.cpp.extra_flags="-D SSID="helloworld"" "-D MY_DEFINE="helloworld""'

This is necessary because the second step that I was talking about above splits the tokens if they're not wrapped.

The only way I have found to get around this tokenization problem is the use of unescaped, nested, double-quote (which should not work, but it does):
$ arduino-cli compile --warnings all --verbose -b arduino:avr:uno \
    --build-property 'compiler.cpp.extra_flags="-D SSID="hello world"" "-D MY_DEFINE="hello world""'
[...]
.../avr-g++ [...] "-D SSID=\"hello world\"" "-D MY_DEFINE=\"hello world\"" [...]

This is actually the correct way to use this flag.

If I attempt to double-quote the entire string on the right side of the compiler.cpp.extra_flags=, the parsing fails:
$ arduino-cli compile --warnings all --verbose -b arduino:avr:uno \
    --build-property 'compiler.cpp.extra_flags="-D SSID="hello world" -D MY_DEFINE="hello world""'
[...]
.../avr-g++ [...] "-D SSID=\"hello world" -D "MY_DEFINE=\"hello" "world\"\"" [...]
avr-g++: error: world"": No such file or directory
It seems like nested-double-quoting only works around a single -D flag.

Same as above, you must wrap each define.

However, the nested double-quote escape breaks down when I try to embed a naked double-quote character inside the SSID, because the parser does not seem to know to skip over the backslashed escaped double-quote. Let's say I want the SSID to be the raw string hello" world. The following does not work:
$ arduino-cli compile --warnings all --verbose -b arduino:avr:uno \
    --build-property 'compiler.cpp.extra_flags="-D SSID="hello\" world"" "-D MY_DEFINE="hello\" world""'
[...]
.../avr-g++ [...] "-D SSID=\"hello\\" "world\"\"" "-D MY_DEFINE=\"hello\\" "world\"\"" [...]
avr-g++: error: world"": No such file or directory
avr-g++: error: world"": No such file or directory

This is an interesting case that I didn't think about, I'll see if it's possible to implement it.

Remarks:

The --build-property flag is definitely better than the previous --build-properties flag. But it seems like the tokenizing and parsing of the -D flags is still not fully accurate. In particular, it does not seem to support string macros with embedded spaces or embedded double-quotes. I think a lot of these problems are caused by multiple levels of tokenizing, and double-quote parsing. If you can clarify and document what those parsing rules are, I think that would help a lot.

I am still of the opinion that exposing the -D flag directly from the arduino-cli go binary is the best way to implement this feature. This would remove a number of confusing layers of parsing, tokenizing and quote and backslash escaping.

I've not investigated the addition of the -D flag, but probably we'd still have the same issues we have with --build-properties or --build-property depending which type we tell cobra to use for that variable.

Another comment: Some of the compile --help examples show the build.extra_flags= property being overridden. But isn't the build.extra_flags property a dangerous one to override? All of the various 3rd party cores that I've looked at define a non-empty value for the build.extra_flags= property. If the end-user overrode this, it is not clear that things would work properly. The compiler.cpp.extra_flags= seems to be the property that is unused by various 3rd party cores, so can be safely overridden by end-users.

I don't know about this, probably someone with more knowledge about this can give a better answer about it. :)

…erties flag

silvanocerza · 2020-11-05T09:42:18Z

Force pushed to resolve conflicts, merging after checks are sucessful.

silvanocerza added component/CLI labels Oct 22, 2020

silvanocerza requested review from cmaglie, ubidefeo and per1234 October 22, 2020 15:26

silvanocerza self-assigned this Oct 22, 2020

cmaglie reviewed Oct 22, 2020

View reviewed changes

silvanocerza changed the title ~~Add support for definitions containing quotes in compile --build-properties flag~~ Deprecate compile --build-properties flag in favor of new featureful --build-property flag Oct 27, 2020

obra mentioned this pull request Oct 28, 2020

[skip changelog] Update contributions guidelines and PR template to add labeling and increase focus on breaking changes #1039

Merged

4 tasks

cmaglie requested changes Oct 28, 2020

View reviewed changes

silvanocerza added 5 commits November 5, 2020 10:32

Add support for definitions containing quotes in compile --build-prop…

96ea15d

…erties flag

Revert back a function

32e0d95

Deprecate --build-properties flag in favor of --build-property

1f7d7f8

Fix some escaping issues on Windows

64930c7

Simplify compile examples

e3507b4

silvanocerza force-pushed the scerza/fix-compile-build-properties branch from f68c808 to e3507b4 Compare November 5, 2020 09:41

silvanocerza merged commit ef4a389 into master Nov 5, 2020

silvanocerza deleted the scerza/fix-compile-build-properties branch November 5, 2020 10:07

obra mentioned this pull request Nov 13, 2020

[feature request] preprocessor define in config (or on command line directly) #1070

Closed

per1234 referenced this pull request in MarginallyClever/Makelangelo-firmware Nov 13, 2020

integrated defines for board and machine

c3a85e4

per1234 mentioned this pull request Dec 29, 2020

build-properties are only usable for compiler flags, not for the general case ArminJo/arduino-test-compile#22

Closed

ianfixes mentioned this pull request Jan 29, 2021

Expose build-properties flag Arduino-CI/arduino_ci#279

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Deprecate compile --build-properties flag in favor of new featureful --build-property flag #1044

Deprecate compile --build-properties flag in favor of new featureful --build-property flag #1044

silvanocerza commented Oct 22, 2020 •

edited

Loading

cmaglie Oct 22, 2020

silvanocerza Oct 22, 2020

cmaglie Oct 22, 2020

cmaglie Oct 22, 2020

silvanocerza commented Oct 27, 2020

ubidefeo commented Oct 27, 2020

silvanocerza commented Oct 27, 2020

silvanocerza commented Oct 27, 2020

obra commented Oct 27, 2020

silvanocerza commented Oct 27, 2020

obra commented Oct 27, 2020

rsora commented Oct 27, 2020

rsora commented Oct 27, 2020

alranel commented Oct 27, 2020

obra commented Oct 28, 2020

cmaglie left a comment

cmaglie Oct 28, 2020

silvanocerza Oct 28, 2020

bxparks commented Oct 28, 2020 •

edited

Loading

bxparks commented Oct 28, 2020

silvanocerza commented Oct 30, 2020

silvanocerza commented Nov 5, 2020

	"List of custom build properties separated by spaces. Or can be used multiple times for multiple properties.")
	"Override a build property with a custom value. Can be used multiple times for multiple properties.")

Deprecate compile --build-properties flag in favor of new featureful --build-property flag #1044

Deprecate compile --build-properties flag in favor of new featureful --build-property flag #1044

Conversation

silvanocerza commented Oct 22, 2020 • edited Loading

cmaglie Oct 22, 2020

Choose a reason for hiding this comment

silvanocerza Oct 22, 2020

Choose a reason for hiding this comment

cmaglie Oct 22, 2020

Choose a reason for hiding this comment

cmaglie Oct 22, 2020

Choose a reason for hiding this comment

silvanocerza commented Oct 27, 2020

ubidefeo commented Oct 27, 2020

silvanocerza commented Oct 27, 2020

silvanocerza commented Oct 27, 2020

obra commented Oct 27, 2020

silvanocerza commented Oct 27, 2020

obra commented Oct 27, 2020

rsora commented Oct 27, 2020

rsora commented Oct 27, 2020

alranel commented Oct 27, 2020

obra commented Oct 28, 2020

cmaglie left a comment

Choose a reason for hiding this comment

cmaglie Oct 28, 2020

Choose a reason for hiding this comment

silvanocerza Oct 28, 2020

Choose a reason for hiding this comment

bxparks commented Oct 28, 2020 • edited Loading

bxparks commented Oct 28, 2020

silvanocerza commented Oct 30, 2020

silvanocerza commented Nov 5, 2020

silvanocerza commented Oct 22, 2020 •

edited

Loading

bxparks commented Oct 28, 2020 •

edited

Loading