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.

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

Add build system support for un-ignore rules #12965

Closed
wants to merge 6 commits into from

Conversation

multiplemonomials
Copy link
Contributor

Summary of changes

Recently I've been migrating code from MBed 2 to MBed OS 5, and one of the real stumbling blocks for me was configuring the .mbedignore properly. For my application, I wanted to disable most extra features, but I needed access to block devices. So, what I wanted was an .mbedignore like this:

# Ignore all extra features (cellular, encryption, storage) by default
components/*
features/*

# Unignore block device library since it's a common utility (and is needed for USB)
!features/storage/blockdevice/*

Unfortunately, MBed's build system doesn't actually have a way to unignore a directory once it's been ignored. So, if I want to enable block devices only, it seems like I need to take a fairly circuitous route. First, I have to create an mbedignore in features that lists all subdirectories except storage, then I have to create an mbedignore in features/storage that lists all subdirectories except blockdevice. This is gross since it requires a huge amount of extra work, and it's also not futureproof since more subdirectories may be added in the future and I'd have to manually update the ignore files.

This PR provides a much better way of handling this by making the ignore file above work how you'd expect it to. This makes it much easier to write ignore files with more complex behavior, and makes it simpler to write a single ignore file in the root dir that does everything you need it to (which is nice so it's easier to update your mbed-os version).

Impact of changes

This change is backwards compatible, with one exception: if anyone has files starting with an exclamation mark listed in their mbedignore, then they will no longer be ignored. If that's an issue we can easily change the escape sequence to something else. I just chose an exclamation mark to be consistent with .gitignore syntax.

This code does somewhat change the way that the resource scanner iterates through the source tree: it no longer skips entering directories that are ignored. Files will still be ignored properly since the ignore rules were already checked for each file before scanning it. This is required since even if a directory is ignored there might be files inside it that are unignored. I was worried that this change might have a performance impact, but I measured the time and it only adds a few hundreths of a second to the scan time for the entire mbed-os folder.

Migration actions required

Documentation

I added a section to the mbedignore page of the docs to describe this change. PR has been submitted here: ARMmbed/mbed-os-5-docs#1313 . Read that for more info about the new syntax.


Pull request type

[] Patch update (Bug fix / Target update / Docs update / Test update / Refactor)
[X] Feature update (New feature / Functionality change / New API)
[] Major update (Breaking change E.g. Return code change / API behaviour change)

Test results

[] No Tests required for this change (E.g docs only update)
[X] Covered by existing mbed-os tests (Greentea or Unittest)
[] Tests / results supplied as part of this PR

Reviewers


@multiplemonomials
Copy link
Contributor Author

Oh also, I'm not quite sure what to do for tests with this. It's covered by existing tests in that they'll make sure existing ignore functionality still works, but they won't test the actual new unignore functionality. How and where should I go about adding tests for this?

@ciarmcom
Copy link
Member

@multiplemonomials, thank you for your changes.
@ARMmbed/mbed-os-tools @ARMmbed/mbed-os-maintainers please review.

@0xc0170
Copy link
Contributor

0xc0170 commented Jun 15, 2020

@ARMmbed/mbed-os-tools Please review

@0xc0170
Copy link
Contributor

0xc0170 commented Jun 15, 2020

Thanks @multiplemonomials

This change is backwards compatible, with one exception: if anyone has files starting with an exclamation mark listed in their mbedignore, then they will no longer be ignored. If that's an issue we can easily change the escape sequence to something else. I just chose an exclamation mark to be consistent with .gitignore syntax.

👍 I would also use !.

This covers a functionality we haven't provided. Tools team will need to review the impact of this. As we are working on updating the tools, we might take this with it, lets wait for their review.

0xc0170
0xc0170 previously approved these changes Jun 15, 2020
@mergify mergify bot added needs: CI and removed needs: review labels Jun 15, 2020
Copy link
Contributor

@rwalton-arm rwalton-arm left a comment

Choose a reason for hiding this comment

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

Looks reasonable to me. I just have a couple of comments about the code style.


filepath_normcase = normcase(file_path)

for regex_index in range(0, len(self._ignore_regexes)):
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we use zip here to tidy this loop up? Then we can say something like:

for ignore_regex, ignore_pattern in zip(self._ignore_regexes, self._ignore_patterns):
    this_regex_match = ignore_regex.match(filepath_normcase)
    ...

This way we have both of the elements there without having to introduce the extra regex_index variable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

heh you can tell I'm mostly a C++ programmer... I had no idea this existed


for regex_index in range(0, len(self._unignore_regexes)):
this_regex_match = self._unignore_regexes[regex_index].match(filepath_normcase)
if this_regex_match:
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be possible to factor out this duplicate logic into a helper function?

# no previous match
ignore_match_pattern = self._ignore_patterns[regex_index]
elif len(self._ignore_patterns[regex_index]) > len(ignore_match_pattern):
# found a longer match
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd prefer to see a comment explaining why a longer match is important, rather than just repeating what the code says.

@mergify mergify bot dismissed 0xc0170’s stale review June 27, 2020 04:39

Pull request has been modified.

@multiplemonomials
Copy link
Contributor Author

OK @rwalton-arm the issues you commented about should be fixed, and I confirmed that the code produces the same results as the previous commit!

@ladislas
Copy link
Contributor

Any news on this?

@adbridge
Copy link
Contributor

adbridge commented Aug 7, 2020

@rwalton-arm @Patater should this be added as a requirement for the new tools now ?

@rwalton-arm
Copy link
Contributor

rwalton-arm commented Aug 7, 2020

Any news on this?

Sorry this has sat for so long. We've been working on a new build system and new tooling. This PR will change files in the legacy tools that are now "frozen". See: https://os.mbed.com/blog/entry/Introducing-the-new-Mbed-Tools/ for more information. We will work out a way to meet these needs in our new build tools.

@multiplemonomials
Copy link
Contributor Author

Alright I suppose I'll try to submit this as a PR to the new tools once they come out for use.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants