Rework particle.include and particle.ignore for CLI compile #646

keeramis · 2023-06-08T14:13:47Z

Description

The CLI cloud compile command has an undocumented pair of files particle.include and particle.ignore that modify which files are sent to the cloud compiler. The implementation is broken because adding files to particle.include strips the relative path.

This PR reworks the usage of particle.include and particle.include files such that the file pattern in those files are used to either include/remove those files from the list of files that is sent to the cloud compiler.

How to Test

Setup particle-cli locally as per the instructions here
Checkout this branch
Run npm start -- compile <platform> </path/to/project>

Adding `particle.include` or `particle.ignore` files

The particle.include and particle.ignore files provide a way to include or exclude additional files from the cloud compiler. Please note that these files are only for special extras and the default files like the source and header files are handled separately and are not affected by the patterns specified in these two files. These files can be located either in the project's root directory or in any of its subdirectories.

When the particle.include file is placed in the root directory, files matching the specified patterns are searched recursively from the root directory. On the other hand, if the particle.include file is placed in a subdirectory, the search for matching files is performed recursively starting from that subdirectory as the base. The system automatically handles duplicates, so there is no need to worry about them.

Similarly, the particle.ignore file follows the same behavior, but instead of including files, it excludes files that match the specified patterns from being sent to the cloud compiler.

Refer to the examples here for particle.include - Example project structure to include files
Refer to the examples here for particle.ignore - Example project structure to ignore files

Related Issues / Discussions

https://app.shortcut.com/particle/story/119000/rework-particle-include-and-particle-ignore-functionality-in-cli-compile

Completeness

keeramis · 2023-06-09T09:50:16Z

src/lib/utilities.js

+			if (os.platform() === 'win32') {
+				// because glob pattern is always in posix format
+				found = found.map(file => path.win32.normalize(file));
+			}


Apparently, glob.sync only works with POSIX formats, which caused it to return POSIX-formatted file paths even on Windows systems. There is a --posix: false that theoritically should help workaround this problem but that option is not working as expected. This has not been a problem in our codebase because the file paths were later processed by path.relative in cloud.js, which handled the necessary adjustments for the specific operating system but when I tried to write unit tests for that specific code, it started to make the tests fail on Windows.

I believe it's best to modify the behavior of glob.sync to generate Windows-formatted paths directly at the source. This way, we can eliminate the need for the caller to handle this issue.

keeramis · 2023-06-09T15:50:32Z

I verified Windows is OK as well! @monkbroc , and it adds the .def files even when the content is in unix format.

This was the particle.include file:

**/*.def

Here's the text from console.

windows-tracker-edge-and-include.txt

eberseth · 2023-06-09T19:15:15Z

src/cmd/cloud.js

+		});
+	}
+
+	_getDefaultIncludes(files, dirname, { followSymlinks }) {
 		// Recursively find source files


Technically speaking, the default list should probably only have the following specific patterns that are found in src/ and lib/src only:

project.properties

src/*.h,hpp,hxx,c,cpp,cxx

src/*.ino

src/build.mk

lib/src/*.h,hpp,hxx,c,cpp,cxx

We don't want to pick up anything in lib/usage/*.ino for example

@eberseth Thanks for your feedback! I've made the suggested modifications. Just to confirm, are there any other significant patterns we should consider, given that we now focus on src/* and lib/src/* directories only, excluding other nested directories? With this, we will also no longer support the flat architecture like this - https://github.com/particle-iot/particle-cli/tree/master/test/__fixtures__/projects/legacy-flat

In summary:

Flat architecture will not be suppoorted - https://github.com/particle-iot/particle-cli/tree/master/test/__fixtures__/projects/legacy-flat

we have this structure of multiple header extensions in one of the tests - should we continue supporting it. https://github.com/particle-iot/particle-cli/tree/master/test/__fixtures__/projects/multiple-header-extensions

another one we until-now support is this 'extended' structure where the helper is with in the /src https://github.com/particle-iot/particle-cli/tree/master/test/__fixtures__/projects/extended

What about symlinks like this? I am changing their structure as well https://github.com/particle-iot/particle-cli/tree/master/test/__fixtures__/projects/symlink

A simple yes/no from you is sufficient for me to make the expected modifications.

No flat format is fine.

We never documented all of the other header file extensions but changing the list will likely break libraries in the future; I suspect that list happened when porting Arduino libraries.

The helper in src structure seems weird at first, but I suspect that is common for anyone who has divided their project into subdirectories, which is probably not that uncommon.

I didn't realize we supported symlinks that way but it could be useful when dealing with shared codebases. But you can also fix this with a build.mk, which might be a better way to do this than symlinks.

Cool. Thanks for the inputs. @rickkas7.

@eberseth To continue supporting certain legacy project structures, I will keep the changes to the original ones I had. To make changes to remove support for legacy formats, I am leaning towards getting the requirements sorted more clearly and will create a separate PR with those changes. For now, let's keep this PR focused on the particle.include and ignore files.

monkbroc

Nice work! I made a few suggestions. You can merge and release after addressing them.

monkbroc · 2023-06-09T19:05:21Z

src/cmd/cloud.js

+		this._getCustomIncludes(files, dirname, { followSymlinks });
+		this._getDefaultIgnores(files, dirname, { followSymlinks });


Put default ignores before custom includes so it's possible to re-include something that got ignored.

monkbroc · 2023-06-09T19:08:27Z

src/cmd/cloud.js

+			if (!globsToInclude || !globsToInclude.length) {
+				continue;
+			}
+			const globList = globsToInclude.map(g => g);


What is thing line for?

monkbroc · 2023-06-09T19:09:23Z

src/cmd/cloud.js

+			}
+			const globList = globsToInclude.map(g => g);
+			const includePaths = utilities.globList(includeDir, globList, { followSymlinks });
+			result.push(...includePaths);


You could just do includePaths.forEach((file) => files.add(file)); instead of putting them in another temporary array.

monkbroc · 2023-06-09T19:26:48Z

src/cmd/cloud.js

+		Array.from(files.values()).sort();
+		files.forEach((file) => {


Interestingly this works but you're not getting the benefits of the sort

Suggested change

Array.from(files.values()).sort();

files.forEach((file) => {

const sortedFiles = Array.from(files.values()).sort();

sortedFiles.forEach((file) => {

eberseth · 2023-06-09T19:39:26Z

src/cmd/cloud.js

-		const includesFile = path.join(dirname, settings.dirIncludeFilename);
-		const ignoreFile = path.join(dirname, settings.dirExcludeFilename);
-		let hasIncludeFile = false;
+		this._getDefaultIncludes(files, dirname, { followSymlinks });


Is the intention to stack the settings on top of each other? For example, the _getDefaultIncludes() will add a bunch of patterns then _getCustomIncludes() will add to the existing pattern list, and so on? Or if the custom list is present it will clear any of the default patterns?

The custom list will not clear the list of files added by the default list. The difference is in files added by the system (_getDefaultIncludes) vs. user (_getCustomIncludes)

Here's what you can expect from this functionality:

_getDefaultIncludes will add the default files as provided in the list into a resultant array that will get sent to the cloud compiler. Default list doesn't look at user inputs. It checks the patterns from the system.

_getCustomIncludes will include more files as per the patterns in the particle.include file as specified by the user. Files added in step1 are not cleared.

_getDefaultIgnores will remove any files as specified by the system (currently only lib/helper/examples/)

_getCustomIgnores will remove any files as specified by the user in the particle.ignore file.

eberseth · 2023-06-09T19:55:46Z

This repo and branch is what should be working for Monitor Edge. There are still files that are not picked up for some reason.
https://github.com/particle-iot-inc/morph-private/tree/feature/cloud-compile

I am using the following command to start the compile:

$ npm start -- compile tracker --target 4.1.0 ~/projects/morph/morph-cloudcomp

It errored out saying that it was missing lib/memfault/src/memfault-firmware-sdk/components/include/memfault/http/http_client.h

src/memfault_demo_http.c:9:10: fatal error: memfault-firmware-sdk/components/include/memfault/http/http_client.h: No such file or directory
    9 | #include "memfault-firmware-sdk/components/include/memfault/http/http_client.h"
      |          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
compilation terminated.

The other files I see missing for some reason are:

lib/memfault/examples/memfault_test/src/memfault_particle_user_config.h
lib/memfault/src/memfault-firmware-sdk/components/include/memfault/core/debug_log.h
lib/memfault/src/memfault-firmware-sdk/components/include/memfault/core/device_info.h
lib/memfault/src/memfault-firmware-sdk/components/include/memfault/core/platform/overrides.h
lib/memfault/src/memfault-firmware-sdk/components/include/memfault/core/platform/reboot_tracking.h
lib/memfault/src/memfault-firmware-sdk/components/include/memfault/http/http_client.h
lib/memfault/src/memfault-firmware-sdk/components/include/memfault/http/utils.h
lib/memfault/src/memfault-firmware-sdk/components/include/memfault/panics/coredump.h

eberseth · 2023-06-09T20:14:27Z

This repo and branch is what should be working for Monitor Edge. There are still files that are not picked up for some reason. https://github.com/particle-iot-inc/morph-private/tree/feature/cloud-compile

I am using the following command to start the compile:
$ npm start -- compile tracker --target 4.1.0 ~/projects/morph/morph-cloudcomp
It errored out saying that it was missing lib/memfault/src/memfault-firmware-sdk/components/include/memfault/http/http_client.h
src/memfault_demo_http.c:9:10: fatal error: memfault-firmware-sdk/components/include/memfault/http/http_client.h: No such file or directory
    9 | #include "memfault-firmware-sdk/components/include/memfault/http/http_client.h"
      |          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
compilation terminated.
The other files I see missing for some reason are:

lib/memfault/examples/memfault_test/src/memfault_particle_user_config.h

lib/memfault/src/memfault-firmware-sdk/components/include/memfault/core/debug_log.h

lib/memfault/src/memfault-firmware-sdk/components/include/memfault/core/device_info.h

lib/memfault/src/memfault-firmware-sdk/components/include/memfault/core/platform/overrides.h

lib/memfault/src/memfault-firmware-sdk/components/include/memfault/core/platform/reboot_tracking.h

lib/memfault/src/memfault-firmware-sdk/components/include/memfault/http/http_client.h

lib/memfault/src/memfault-firmware-sdk/components/include/memfault/http/utils.h

lib/memfault/src/memfault-firmware-sdk/components/include/memfault/panics/coredump.h

Nevermind

rickkas7

I didn't go through the code or run it, but the concept looks good. When is this planned for release so I can update the docs?

keeramis · 2023-06-12T10:03:39Z

@rickkas7 Currently my only ask for feedback is for this - #646 (comment)

Once I get clarification on the above, I will get this into the next CLI release.

keeramis added 6 commits June 8, 2023 09:26

Test helper

c81f495

[tests] get default files

df1c33e

Add _getCustomIncludes to include files from particle.include

f1958d1

Add methods to ignore files

20e3aac

Refactor

85cd050

Refactor to use Set instead of List

8e3bc78

keeramis changed the title ~~Rework 'particle.include' and 'particle.ignore' functionality in CLI compile~~ Rework 'particle.include'/'particle.ignore' functionality in CLI compile Jun 8, 2023

keeramis changed the title ~~Rework 'particle.include'/'particle.ignore' functionality in CLI compile~~ Rework particle.include/particle.ignore functionality in CLI compile Jun 8, 2023

keeramis changed the title ~~Rework particle.include/particle.ignore functionality in CLI compile~~ Rework particle.include and particle.ignore for CLI compile Jun 8, 2023

keeramis force-pushed the bug/sc-119000/project-incl-files branch from 4f064eb to a095853 Compare June 8, 2023 19:54

[test] Modify e2e tests and add support for windows path to glob.sync

9907954

keeramis force-pushed the bug/sc-119000/project-incl-files branch from a317de1 to 9907954 Compare June 9, 2023 09:32

keeramis commented Jun 9, 2023

View reviewed changes

keeramis requested review from monkbroc and eberseth June 9, 2023 09:51

minor

ddcdb7e

eberseth reviewed Jun 9, 2023

View reviewed changes

Allow compiling library examples with more than 1 file

ae25437

monkbroc approved these changes Jun 9, 2023

View reviewed changes

eberseth reviewed Jun 9, 2023

View reviewed changes

eberseth approved these changes Jun 9, 2023

View reviewed changes

keeramis added the DO-NOT-MERGE label Jun 11, 2023

keeramis requested a review from rickkas7 June 12, 2023 08:48

rickkas7 approved these changes Jun 12, 2023

View reviewed changes

keeramis force-pushed the bug/sc-119000/project-incl-files branch from 29ace01 to ae25437 Compare June 12, 2023 16:07

Address review comments

eeac2ca

keeramis removed the DO-NOT-MERGE label Jun 13, 2023

keeramis merged commit 27c20a3 into master Jun 13, 2023

monkbroc deleted the bug/sc-119000/project-incl-files branch January 19, 2024 19:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rework particle.include and particle.ignore for CLI compile #646

Rework particle.include and particle.ignore for CLI compile #646

keeramis commented Jun 8, 2023 •

edited

Loading

keeramis Jun 9, 2023 •

edited

Loading

keeramis commented Jun 9, 2023

eberseth Jun 9, 2023 •

edited

Loading

keeramis Jun 12, 2023 •

edited

Loading

rickkas7 Jun 12, 2023

keeramis Jun 12, 2023

keeramis Jun 12, 2023

monkbroc left a comment

monkbroc Jun 9, 2023

monkbroc Jun 9, 2023

monkbroc Jun 9, 2023

monkbroc Jun 9, 2023

eberseth Jun 9, 2023

keeramis Jun 11, 2023 •

edited

Loading

eberseth commented Jun 9, 2023

eberseth commented Jun 9, 2023

rickkas7 left a comment

keeramis commented Jun 12, 2023

		this._getCustomIncludes(files, dirname, { followSymlinks });
		this._getDefaultIgnores(files, dirname, { followSymlinks });

Rework particle.include and particle.ignore for CLI compile #646

Rework particle.include and particle.ignore for CLI compile #646

Conversation

keeramis commented Jun 8, 2023 • edited Loading

Description

How to Test

Adding particle.include or particle.ignore files

Related Issues / Discussions

Completeness

keeramis Jun 9, 2023 • edited Loading

Choose a reason for hiding this comment

keeramis commented Jun 9, 2023

eberseth Jun 9, 2023 • edited Loading

Choose a reason for hiding this comment

keeramis Jun 12, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

monkbroc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

keeramis Jun 11, 2023 • edited Loading

Choose a reason for hiding this comment

eberseth commented Jun 9, 2023

eberseth commented Jun 9, 2023

rickkas7 left a comment

Choose a reason for hiding this comment

keeramis commented Jun 12, 2023

keeramis commented Jun 8, 2023 •

edited

Loading

Adding `particle.include` or `particle.ignore` files

keeramis Jun 9, 2023 •

edited

Loading

eberseth Jun 9, 2023 •

edited

Loading

keeramis Jun 12, 2023 •

edited

Loading

keeramis Jun 11, 2023 •

edited

Loading