Skip to content

Sketch compiling via commandline does not check for valid primary .ino filename #10755

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

Open
ArcanoxDragon opened this issue Sep 13, 2020 · 9 comments
Labels
arduino-builder The tool used to handle the Arduino sketch compilation process arduino-cli Related to the arduino-cli tool Type: Bug

Comments

@ArcanoxDragon
Copy link

When the Arduino IDE is invoked via the command line for a Verify or Upload task, such as when an external tool like microsoft/vscode-arduino invokes a build, but the name of the sketch file provided on the command line does not match its containing folder name, the first file from FileUtils.listFiles is used as the "primary" file, resulting in undefined behavior (the order returned by listFiles seems non-deterministic) and apparent obscure bugs such as the one I filed at microsoft/vscode-arduino#1100.

The requirement to have the .ino file match the folder name seems arbitrary and restrictive for command-line compilations when the IDE itself is not being used for development. I developed a simple fix for this on my own branch and plan on filing a PR.

@matthijskooijman
Copy link
Collaborator

The requirement to have the .ino file match the folder name seems arbitrary and restrictive for command-line compilations when the IDE itself is not being used for development. I developed a simple fix for this on my own branch and plan on filing a PR.

It's not just an arbitrary restriction, the .ino-file-that-matches-the-directory is also the primary file that should be the first when joining multiple .ino files, so it does actually influence the compilation result.

I was going to write that not having a "primary" file would make it impossible to have a specific file as the "first" file, but things could just fall back to sorting all .ino files alphabetically or so and then that would be the order. If you do care about one file being primary, then you can just name it after the folder.

One caveat is that if someone renames the folder, but not the primary .ino file, then the compilation might break due to reordering (currently, the sketch fails to load, which is probably slightly better).

As for the compilation itself, this is done by arduino-builder now (and will be done by arduino-cli in the future), so if those tools still require a .ino file to match the directory name, then the IDE should probably enforce this as well. Or do those tools allow compiling a sketch directory where no .ino files matches already? If so, then the IDE should probably drop this check indeed.

@matthijskooijman
Copy link
Collaborator

One more thought: In your PR, you make the .ino file passed on the commandline the "primary" one, but that seems a bit fragile: Depending on what .ino file you pass, the compilation result might change. Also, when you have an arbitrary sketch with multiple .ino files, then you would have to remember which of the .ino files is supposed to be the primary one to make the sketch work.

If this restriction is dropped, I would rather suggest that when there is no matching .ino file, there is simply no primary file at all and compilation should just treat all .ino files equally (and probably in alphabetical order). But again, the behavior of arduino-builder in this area seems also relevant. The bug you report here is not so much about this restriction, but a bug that causes a non-.ino file to be passed to arduino-builder I think.

@cmaglie
Copy link
Member

cmaglie commented Sep 14, 2020

Or do those tools allow compiling a sketch directory where no .ino files matches already? If so, then the IDE should probably drop this check indeed.

They don't, and there is no such feature planned. The problem arises with sketch made by multiple .ino files as you already explained. BTW I see this issue coming back from time to time maybe we should document better that the "sketch" is not just the single .ino file but the whole folder?

We may like it or not, but this is how it worked from day 0, doesn't look like a good idea to change this behavior now.

@matthijskooijman
Copy link
Collaborator

that the "sketch" is not just the single .ino file but the whole folder?

I wasn't suggesting to change that, just to not enforce that there is a primary .ino file. AFAIK, currently the compilation order of .ino files is:

  1. The primary file, named after the directory
  2. The other .ino files, in alphabetical order (I think)

It might be easy to just make 1. optional and if it is not present, just use all .ino files in alphabetical order? That should be 100% backward compatible, any sketches that are currently supported are still supported, you just add some more options.

If we choose not to relax this requirement, then the commandline --verify should probably be still be fixed, since it currently picks a random, potentially non-.ino, file as the primary file and passes that to arduino-builder, possibly breaking the build. The easiest way to fix this is to add the same check (just reject sketches without a primary .ino file).

Or do those tools allow compiling a sketch directory where no .ino files matches already? If so, then the IDE should probably drop this check indeed.

I just tested this, it seems that arduino-builder does not check this, it just compiles fine as long as you pass a .ino file on the commandline. arduino-cli does check this and complains.

@cmaglie
Copy link
Member

cmaglie commented Sep 14, 2020

I just tested this, it seems that arduino-builder does not check this, it just compiles fine as long as you pass a .ino file on the commandline. arduino-cli does check this and complains.

the arduino-builder it's missing the check (and consequently the "IDE via command line" that uses the arduino-builder). This should be fixed, indeed. We are deprecating arduino-builder in favor of arduino-cli (and we will replace it hopefully soon in the main release).

I wasn't suggesting to change that, just to not enforce that there is a primary .ino file. AFAIK, currently the compilation order of .ino files is:

  • The primary file, named after the directory
  • The other .ino files, in alphabetical order (I think)

my main concern is that if you try to open an "invalid" sketch with the IDE GUI you get a dialog like this:

image

so if we do as you suggest:

  1. we will create a discrepancy in what the IDE accepts via GUI, what the IDE accepts via command-line that is always a bad thing
  2. we will add an unwanted side-effect: if the user changes the name of the sketch folder it may cause the sketch to not compile anymore without any warning
  3. we must make all tools do the same thing, arduino-cli in primis because it will become our reference implementation, so the patch should start from there.

As I said, this issue comes back from time to time, but it's trickier than it seems. If we really want to fix this I suggest we move the discussion in the arduino-cli repo to find the less disruptive solution.

@matthijskooijman
Copy link
Collaborator

My suggestion would be to simply drop this limitation everywhere, so the GUI prompt you mention in 1. would be dropped, and of course arduino-cli would also need to be changed, which solves 3.

Number 2 is indeed an issue, though one could argue that it's a small inconvenience in exchange for more freedom in naming sketches.

Agreed that this is not the ideal place to discuss this change, this issue should probably be about the missing check in the IDE now. Even though I'm not super-interested in changing this policy, it seems there have been some useful arguments in this discussion, so I've created a separate issue for this: arduino/arduino-cli#948

@matthijskooijman
Copy link
Collaborator

@briman0094, I've closed your PR as I think that is not the correct approach here (see the PR for detailed comments).

I can see two ways forward:

  1. Lift the requirement to have a file named after the directory and/or provide an alternative "Main.ino" or similar. This is discussed in Consider lifting limitation that primary ino file must be named after directory arduino-cli#948, so I'll not go into this in detail, except noting that a proper implementation of that change should keep in mind to fix this issue as well.
  2. If we do not make the above change (or until we do), the code path for --verify should be modified to check for a valid primary .ino file, named after the directory, and reject the sketch if it is not. This makes it consistent with the GUI and arduino-cli behavior.

@ArcanoxDragon
Copy link
Author

Perhaps it's best to throw a clear error from the command line then, instead of letting the current undeterministic behavior cause random compile errors due to a random file being picked as the "entry point". While trying to figure out what was going on, I stumbled across quite a few threads on the Arduino forums in which it was recommended to not have multiple .ino files in a sketch folder (I think I even saw someone mention that it was not supported) and to have all code that's not in the main .ino in a .h or a .cpp file. I assumed that when compiling and providing an explicit .ino file path, that should be treated as the definite "entry point" for determining the order of all includes, hence changing the .ino file provided definitely would change the compilation result.

I guess that's not the point of multiple .ino files however. I've started looking into the leka/Arduino-Makefile project instead so that I can organize my code better and have multiple "main" files which target different boards while sharing the same code, as it appears this is not an apparent goal of the Arduino IDE and compiler themselves, at least not right now.

@matthijskooijman
Copy link
Collaborator

I assumed that when compiling and providing an explicit .ino file path, that should be treated as the definite "entry point" for determining the order of all includes, hence changing the .ino file provided definitely would change the compilation result.

This is indeed not how things were intended. I guess the usecase in itself is interesting to explore, but that should then be the subject of a new issue (probably in arduino-cli as well).

@matthijskooijman matthijskooijman changed the title Incorrect "primary" file chosen when compiling via command line Sketch compiling via commandline does not check for valid primary .ino filename Sep 14, 2020
@per1234 per1234 added arduino-builder The tool used to handle the Arduino sketch compilation process arduino-cli Related to the arduino-cli tool Type: Bug labels Sep 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
arduino-builder The tool used to handle the Arduino sketch compilation process arduino-cli Related to the arduino-cli tool Type: Bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants