feat(node): Keep created node_modules directories until after resolution

[sschuberth]

Please have a look at the individual commit messages for the details.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 68.11%. Comparing base (e77fadf) to head (8179dfb).
Report is 6 commits behind head on main.

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #9781      +/-   ##
============================================
+ Coverage     68.08%   68.11%   +0.02%     
- Complexity     1286     1287       +1     
============================================
  Files           249      249              
  Lines          8829     8837       +8     
  Branches        918      918              
============================================
+ Hits           6011     6019       +8     
  Misses         2432     2432              
  Partials        386      386              

Flag	Coverage Δ
funTest-docker	65.15% <100.00%> (+0.14%)	⬆️
funTest-non-docker	33.35% <ø> (ø)
test-ubuntu-24.04	35.78% <0.00%> (-0.07%)	⬇️
test-windows-2022	35.76% <0.00%> (-0.07%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[fviernau]

I still have to think about this commit a bit, haven't finished the review.

The previous commits LGTM now. Do you want to split out this commit so you can merge the rest ?

[sschuberth]

Do you want to split out this commit so you can merge the rest ?

I'm not sure... since this is (to me) the most important commit that concludes the series and allows for the React-Native NPM / Gradle use-case to be analyzed, there is not much value in merging the other commits without this.

I still have to think about this commit a bit

Maybe it helps if you look at the current implementation of stashDirectories(): It's basically just a shortcut for creating a DirectoryStash instance, that is then closed via use {}. All that the last commit intends to do is to split the creation of the DirectoryStash instance and the closing across beforeResolution() / afterResolution() by calling close() explicitly instead of leveraging use {}.

[fviernau]

Maybe it helps if you look at the current implementation of stashDirectories()

I got that bit, but I need to first better understand some details of the analyzer, e.g. how this fits with mapDefinitionFiles, but also in general.

[sschuberth]

Could we also address issues, if any, as a follow-up? I'd like to include this feature into tomorrow's ORT release.

Regarding the mapping of definition files: That happens in findManagedFiles() before analyze{InParallel}() is called. So {before,after}Resolution() see the mapped files, just like originally, if that was one of the questions.

[fviernau]

I'm a bit short in time before the release, and I'm not 100% confident yet about the approach of the top-most commit (I'd need do research). In general, I believe it'd be nice to also have @mnonnenmacher's view.

[sschuberth]

In general, I believe it'd be nice to also have @mnonnenmacher's view.

Or also @MarcelBochtler. Whatever it takes to move this forward.

[sschuberth]

The previous commits LGTM now. Do you want to split out this commit so you can merge the rest ?

Despite my original plan, I've done that now to move things forward. Please approve @fviernau: #9824

[mnonnenmacher]

In general, I believe it'd be nice to also have @mnonnenmacher's view.

Looks good to me, I don't see any issues with the approach.

[mnonnenmacher]

In general, I believe it'd be nice to also have @mnonnenmacher's view.

Looks good to me, I don't see any issues with the approach.

Actually, no actionable changes were requested. The review was just blocked out of a lack of time to think about it.