Skip to content
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.

Sign up for GitHub

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

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

Merged
merged 10 commits into from
Jun 13, 2023
Merged

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

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
c81f495
Test helper
keeramis Jun 8, 2023
df1c33e
[tests] get default files
keeramis Jun 8, 2023
f1958d1
Add _getCustomIncludes to include files from particle.include
keeramis Jun 8, 2023
20e3aac
Add methods to ignore files
keeramis Jun 8, 2023
85cd050
Refactor
keeramis Jun 8, 2023
8e3bc78
Refactor to use Set instead of List
keeramis Jun 8, 2023
9907954
[test] Modify e2e tests and add support for windows path to glob.sync
keeramis Jun 8, 2023
ddcdb7e
minor
keeramis Jun 9, 2023
ae25437
Allow compiling library examples with more than 1 file
monkbroc Jun 9, 2023
eeac2ca
Address review comments
keeramis Jun 12, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 0 additions & 3 deletions settings.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,9 +37,6 @@ let settings = {
],
showIncludedSourceFiles: true,

dirIncludeFilename: 'particle.include',
dirExcludeFilename: 'particle.ignore',

cloudKnownApps: {
'tinker': true
},
Expand Down
87 changes: 56 additions & 31 deletions src/cmd/cloud.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -749,7 +749,7 @@ module.exports = class CloudCommand extends CLICommandBase {
continue;
}

if (!alwaysIncludeThisFile && settings.notSourceExtensions.includes(ext)){
if (!alwaysIncludeThisFile && settings.notSourceExtensions.includes(ext)){ // not source files
continue;
}

Expand Down Expand Up @@ -779,11 +779,24 @@ module.exports = class CloudCommand extends CLICommandBase {
*/
_processDirIncludes(fileMapping, dirname, { followSymlinks } = {}){
dirname = path.resolve(dirname);
let files = new Set();

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

@keeramis keeramis Jun 11, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. _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.
  2. _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.
  3. _getDefaultIgnores will remove any files as specified by the system (currently only lib/helper/examples/)
  4. _getCustomIgnores will remove any files as specified by the user in the particle.ignore file.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

this._getCustomIgnores(files, dirname, { followSymlinks });

// Add files to fileMapping
Array.from(files.values()).sort();
files.forEach((file) => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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) => {

// source relative to the base directory of the fileMapping (current directory)
const source = path.relative(fileMapping.basePath, file);
const target = path.relative(dirname, file);
fileMapping.map[target] = source;
});
}

_getDefaultIncludes(files, dirname, { followSymlinks }) {
// Recursively find source files
Copy link

@eberseth eberseth Jun 9, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@keeramis keeramis Jun 12, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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:

  1. Flat architecture will not be suppoorted - https://github.com/particle-iot/particle-cli/tree/master/test/__fixtures__/projects/legacy-flat
  2. 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
  3. 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
  4. 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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool. Thanks for the inputs. @rickkas7.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

let includes = [
'**/*.h',
Expand All @@ -793,45 +806,57 @@ module.exports = class CloudCommand extends CLICommandBase {
'**/*.ino',
'**/*.cpp',
'**/*.c',
'**/build.mk',
'project.properties'
];

if (fs.existsSync(includesFile)){
//grab and process all the files in the include file.
const result = utilities.globList(dirname, includes, { followSymlinks });
result.forEach((file) => files.add(file));
}

includes = utilities.trimBlankLinesAndComments(
utilities.readAndTrimLines(includesFile)
);
hasIncludeFile = true;
_getCustomIncludes(files, dirname, { followSymlinks }) {
const includeFiles = utilities.globList(dirname, ['**/particle.include'], { followSymlinks });
const result = [];

for (const includeFile of includeFiles) {
const includeDir = path.dirname(includeFile);
const globsToInclude = utilities.trimBlankLinesAndComments(utilities.readAndTrimLines(includeFile));
if (!globsToInclude || !globsToInclude.length) {
continue;
}
const globList = globsToInclude.map(g => g);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is thing line for?

const includePaths = utilities.globList(includeDir, globList, { followSymlinks });
result.push(...includePaths);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

let files = utilities.globList(dirname, includes, { followSymlinks });
result.forEach((file) => files.add(file));
}

if (fs.existsSync(ignoreFile)){
const ignores = utilities.trimBlankLinesAndComments(
utilities.readAndTrimLines(ignoreFile)
);
_getDefaultIgnores(files, dirname, { followSymlinks }) {
// Recursively find default ignore files
let ignores = [
'lib/*/examples/**/*.*'
];

const ignoredFiles = utilities.globList(dirname, ignores, { followSymlinks });
files = utilities.compliment(files, ignoredFiles);
}
const result = utilities.globList(dirname, ignores, { followSymlinks });
result.forEach((file) => files.delete(file));
}

// Add files to fileMapping
files.forEach((file) => {
// source relative to the base directory of the fileMapping (current directory)
const source = path.relative(fileMapping.basePath, file);
_getCustomIgnores(files, dirname, { followSymlinks }) {
const ignoreFiles = utilities.globList(dirname, ['**/particle.ignore'], { followSymlinks });
const result = [];

// If using an include file, only base names are supported since people are using those to
// link across relative folders
let target;
if (hasIncludeFile){
target = path.basename(file);
} else {
target = path.relative(dirname, file);
for (const ignoreFile of ignoreFiles) {
const ignoreDir = path.dirname(ignoreFile);
const globsToIgnore = utilities.trimBlankLinesAndComments(utilities.readAndTrimLines(ignoreFile));
if (!globsToIgnore || !globsToIgnore.length) {
continue;
}
fileMapping.map[target] = source;
});
const globList = globsToIgnore.map(g => g);
const ignoredPaths = utilities.globList(ignoreDir, globList, { followSymlinks });
result.push(...ignoredPaths);
}
result.forEach((file) => files.delete(file));
}


Expand Down
Loading