Fix casing of assembly sketch files

[silvanocerza]

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)
• UPGRADING.md has been updated with a migration guide (for breaking changes)

• What kind of change does this PR introduce?

Fixes a bug with sketch files parsing.

• What is the current behavior?

Both .s and .S extensions are accepted as Sketch assembly files.

• What is the new behavior?

Now only .S extensions are accepted as Sketch files.

• Does this PR introduce a breaking change, and is
  titled accordingly?

Nope.

• Other information:

This will also enforce lowercase for other files extensions.

---

See how to contribute

[matthijskooijman]

Why this change? Isn't lowercase .s a commonly used extension for assembly files (maybe less common than .S, though)?

Also, AFAICS this does not change just the .S files, but makes all file extensions case sensitive where they were case insensitive before.

Also, I disagree that this is not a breaking change, since this can make sketches that used to work now fail to compile when they use lowercase .s, or somewhat less standard uppercase other extensions (e.g. .CPP).

Also, if this is changed, I think that all IDEs should be changed accordingly to no longer load files that will not be compiled? Then at least things are consistent and less surprising for the user (but I'm not in favor of changing this).

[silvanocerza]

Why this change? Isn't lowercase .s a commonly used extension for assembly files (maybe less common than .S, though)?

This change stems form this issue: arduino/arduino-ide#218
I have no idea about the casing of assembly files.

Also, AFAICS this does not change just the .S files, but makes all file extensions case sensitive where they were case insensitive before.

Yup.

Also, I disagree that this is not a breaking change, since this can make sketches that used to work now fail to compile when they use lowercase .s, or somewhat less standard uppercase other extensions (e.g. .CPP).

I didn't mark it as a breaking change just because it was categorized as a bug otherwise I'd have done it.

Anyway paging @per1234, @ubidefeo and @cmaglie. Want to hear their opinion about this.

I can drop this PR we don't like this change.

[ubidefeo]

@silvanocerza
I would just make sure that they are supported, and would disregard the casing.
forcing casing can sometimes lead to issues and like @per1234 said yesterday sometimes we find .s and some others we find .S

[per1234]

The problem is that the Java Arduino IDE only compiles assembly files if the extension is .S. This is the way it has always been. So there are two problems:

• Arduino IDE 2.x actively prevents people from creating specification compliant sketches.
• Arduino CLI allows people to create sketches that are incompatible with the Java Arduino IDE

I'm not against adding support for .s in addition to .S, but I also think it's important to provide consistent compatibility across all official development software. I don't want to see a situation created where a developer says "you can only use my sketch with Arduino CLI". So I would like to see things like this done universally all at once, rather than in a haphazard fashion as each developer feels like deviating from the specification.

The general approach to something like that might be to making the necessary changes in the code, but controlled via a "feature toggle". Only once the support has been added to all the development software (Arduino IDE, Arduino CLI, Arduino Web Editor), are the releases made with the feature enabled.

[cmaglie]

At a first glance I had the same thinking as @matthijskooijman but looking closely at the report from the issue arduino/arduino-ide#218 the user says .s files are not compiled.
I tested it and I can confirm it:

~/Arduino/Blink$ arduino-cli compile -b arduino:avr:uno
Sketch uses 936 bytes (2%) of program storage space. Maximum is 32256 bytes.
Global variables use 9 bytes (0%) of dynamic memory, leaving 2039 bytes for local variables. Maximum is 2048 bytes.

~/Arduino/Blink$ echo xxxxx > test.s
~/Arduino/Blink$ arduino-cli compile -b arduino:avr:uno
Sketch uses 936 bytes (2%) of program storage space. Maximum is 32256 bytes.
Global variables use 9 bytes (0%) of dynamic memory, leaving 2039 bytes for local variables. Maximum is 2048 bytes.

~/Arduino/Blink$ mv test.s test.S
~/Arduino/Blink$ arduino-cli compile -b arduino:avr:uno
/home/cmaglie/Arduino/Blink/test.S: Assembler messages:
/home/cmaglie/Arduino/Blink/test.S:1: Error: unknown opcode `xxxxx'

Error during build: exit status 1
~/Arduino/Blink$ 

The same happens with .cpp vs .CPP (the latter is not compiled).

So it seems that the actual selection of files to compile is not done in the code touched by this PR and this patch is really not breaking.

[cmaglie]

• Arduino IDE 2.x actively prevents people from creating specification compliant sketches.
• Arduino CLI allows people to create sketches that are incompatible with the Java Arduino IDE

I can't agree more.
Also, as I said in my last comment, the Arduino CLI actually allows only sketches that are compatible with the Arduino IDE 1.8 (and thinking about it, it could not be otherwise since arduino-builder is based on the arduino-cli!)

So the way forward is to change the Arduino IDE 2.0 to match the other tools, I know that there is a gRPC service used by the IDE 2.0 to get the list of files to open and I think this is the purpose of this PR. Can you confirm it @silvanocerza?
If the answer is yes for me this PR is ready to merge.

[silvanocerza]

I confirm this changes the gRPC interface.

I used a sketch containing this files to test:

hello
├── bar.s
├── foo.S
└── hello.ino

This is before the changes of this PR:

2021/03/23 12:17:54 calling Version
2021/03/23 12:17:54 arduino-cli version: git-snapshot
2021/03/23 12:17:54 calling LoadSketch
2021/03/23 12:17:54 Sketch main file: /home/alien/workspace/arduino-cli/client_example/hello/hello.ino
2021/03/23 12:17:54 Sketch location: /home/alien/workspace/arduino-cli/client_example/hello
2021/03/23 12:17:54 Other sketch files: []
2021/03/23 12:17:54 Sketch additional files: [/home/alien/workspace/arduino-cli/client_example/hello/bar.s /home/alien/workspace/arduino-cli/client_example/hello/foo.S]

this is after:

2021/03/23 12:17:26 calling Version
2021/03/23 12:17:26 arduino-cli version: git-snapshot
2021/03/23 12:17:26 calling LoadSketch
2021/03/23 12:17:26 Sketch main file: /home/alien/workspace/arduino-cli/client_example/hello/hello.ino
2021/03/23 12:17:26 Sketch location: /home/alien/workspace/arduino-cli/client_example/hello
2021/03/23 12:17:26 Other sketch files: []
2021/03/23 12:17:26 Sketch additional files: [/home/alien/workspace/arduino-cli/client_example/hello/foo.S]

As you can see the bar.s file is not returned anymore.

[cmaglie]

For the record, here is where the actual build file selection happens:

arduino-cli/legacy/builder/builder_utils/utils.go

Lines 69 to 82 in 0ddb024

	func CompileFiles(ctx *types.Context, sourcePath *paths.Path, recurse bool, buildPath *paths.Path, buildProperties *properties.Map, includes []string) (paths.PathList, error) {
	sSources, err := findFilesInFolder(sourcePath, ".S", recurse)
	if err != nil {
	return nil, errors.WithStack(err)
	}
	cSources, err := findFilesInFolder(sourcePath, ".c", recurse)
	if err != nil {
	return nil, errors.WithStack(err)
	}
	cppSources, err := findFilesInFolder(sourcePath, ".cpp", recurse)
	if err != nil {
	return nil, errors.WithStack(err)
	}

the function findFilesInFolder does a case-sensitive match, that's why the change in this PR does not affect compile.

[matthijskooijman]

Bummer, merged before I finished my (overly long, as often) response... I'll post it below anyway as it has some thoughts on followup actions. The only objection I had to the PR that is now merged is the commit message could have been more clear, but well, that's in the past now :-)

This change stems form this issue: arduino/arduino-ide#218

Thanks for providing the issue link, that helps understand where this comes from :-)

So there's a separate list for "copy into build dir" (which is now case insensitive) and "apply this recipe to these extensions" (which I presume is now case insensitive). If that's the case, then this commit really unifies the "copy into build dir" behavior" with the "apply recipe" behavior, which makes sense to me, but the commit message should probably be updated to reflect this (and link to the issue as well, I'd say) before merging then.

Note that if this is the case, there is still a small compatibility issue: I expect that currently you could #include "foo.H (or more unlikely, #include "foo.CPP") and that would work, but with this change, if file copying is now case sensitive, that would no longer work (haven't really checked). I don't think this much of a problem or something that would occur in practice, but it might be considered a small breaking change.

I would just make sure that they are supported, and would disregard the casing.

This is another approach, just make everything case insensitive, which would probably require changes in the IDEs as well to match. This would also be a reasonable option IMHO, which could reduce user surprise.

I also vaguely recall another related issue or PR not so long ago that also considered case sensitivity of extensions and where we also identified these two separate lists, and considered to unify them but I can't quite find this anymore... In any case, it might be something to consider: adding a global extension-to-recipe map, modify compileFiles() to use it and then maybe also use that same map to auto-add entries to SourceFilesValidExtensions and AdditionalFileValidExtensions. That would still require changes to make the case-sensitivity handling consistent between all code that matches these extensions.

Also: If we're touching these lists, and maybe also need matching changes in the IDEs, maybe it would be good to review the list of extensions and add additional ones (e.g. #1149 and https://github.com/arduino/arduino-builder/issues/268)

[per1234]

I'll post it below anyway as it has some thoughts on followup actions.

Thanks! Your input is invariably excellent.

another related issue or PR not so long ago that also considered case sensitivity of extensions and where we also identified these two separate lists, and considered to unify them but I can't quite find this anymore

I remember it also. Here it is:
#707 (comment)