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

Stop using /p:RestoreForceEvaluate=true in CI #12004

Open
jonthysell opened this issue Aug 10, 2023 · 3 comments
Open

Stop using /p:RestoreForceEvaluate=true in CI #12004

jonthysell opened this issue Aug 10, 2023 · 3 comments
Labels
Area: Compliance bug security Pull requests that address a security vulnerability Workstream: ES Compliance SFI Provide regular ES infrastructure and ensur RNW meets internal security and compliance requirements.
Milestone

Comments

@jonthysell
Copy link
Contributor

Problem Description

The whole point of using (NuGet) dependency lock files is to ensure reliable builds by locking to the dependencies in the lock file.

However, we let the PR/CI re-evaluate the dependencies at build time, which is a big no-no. Worst case scenario we download and use a hijacked dependency package, ignoring that the version/hash doesn't match what's in our (trusted) lock file.

Steps To Reproduce

See PR/CI issues such as #11998 for examples of PR/CI using dependencies we didn't expect.

Expected Results

No response

CLI version

npx react-native -v

Environment

npx react-native info

Target Platform Version

None

Target Device(s)

Desktop

Visual Studio Version

Visual Studio 2022

Build Configuration

None

Snack, code example, screenshot, or link to a repository

No response

@jonthysell jonthysell added bug security Pull requests that address a security vulnerability Area: Compliance labels Aug 10, 2023
@microsoft-github-policy-service microsoft-github-policy-service bot added the Needs: Triage 🔍 New issue that needs to be reviewed by the issue management team (label applied by bot) label Aug 10, 2023
@jonthysell
Copy link
Contributor Author

Note, we can't just turn this off because of our mixing of C## and C++ projects. From #11998 (comment):

... because we have UWP C# projects (such as Microsoft.ReactNative.Managed) that depend on C++/WinRT projects (such as Microsoft.ReactNative), and we're using PackageReferences in both, NuGet restore always fails if we don't use force evaluate. We get these errors:

D:\a\_work\1\s\vnext\Microsoft.ReactNative.Managed\Microsoft.ReactNative.Managed.csproj : error NU1004: The project Microsoft.ReactNative has no compatible target framework. The packages lock file is inconsistent with the project dependencies so restore can't be run in locked mode. Disable the RestoreLockedMode MSBuild property or pass an explicit --force-evaluate option to run restore to update the lock file. [D:\a\_work\1\s\vnext\Microsoft.ReactNative.sln]
D:\a\_work\1\s\vnext\Microsoft.ReactNative.Managed.UnitTests\Microsoft.ReactNative.Managed.UnitTests.csproj : error NU1004: The project Microsoft.ReactNative has no compatible target framework. The packages lock file is inconsistent with the project dependencies so restore can't be run in locked mode. Disable the RestoreLockedMode MSBuild property or pass an explicit --force-evaluate option to run restore to update the lock file. [D:\a\_work\1\s\vnext\Microsoft.ReactNative.sln]
D:\a\_work\1\s\vnext\Microsoft.ReactNative.Managed.IntegrationTests\Microsoft.ReactNative.Managed.IntegrationTests.csproj : error NU1004: The project Microsoft.ReactNative has no compatible target framework. The packages lock file is inconsistent with the project dependencies so restore can't be run in locked mode. Disable the RestoreLockedMode MSBuild property or pass an explicit --force-evaluate option to run restore to update the lock file. [D:\a\_work\1\s\vnext\Microsoft.ReactNative.sln]

This is a bug in VS/NuGet. Related issues/comments:

So we're stuck in a spot where we have to force the evaluation on every restore, even if everything is correct and we're happy with the state of our lock files.

@acoates-ms acoates-ms removed the Needs: Triage 🔍 New issue that needs to be reviewed by the issue management team (label applied by bot) label Aug 14, 2023
@acoates-ms acoates-ms added this to the 0.73 milestone Aug 14, 2023
@acoates-ms
Copy link
Contributor

Is this something you are going to look at @jonthysell or should @JunielKatarn look into it?

@jonthysell jonthysell modified the milestones: 0.73, Next Nov 7, 2023
@jonthysell jonthysell modified the milestones: Next, Backlog May 22, 2024
@jonthysell
Copy link
Contributor Author

This is technically important, but we can't really implement this without ONE of the following:

  1. Every build requests the same dependencies even if they don't use them (i.e. no variations where projects do/don't use WinAppSDK)
  2. We only have one set of binaries and therefore one set of dependencies (i.e. retire react-native-win32, RNW Paper, etc).

Putting this on the backlog until we get some kind of alert.

@jonthysell jonthysell removed their assignment May 22, 2024
jonthysell added a commit that referenced this issue Sep 9, 2024
## Description

SourceLink is needed to ensure that our NuGet packages have matching source information when symbols are resolved. So we have an `EnableSourceLink` property which tells some of our projects to add source link, and we enable that when doing CI/PR/official builds by setting the property to true when we call msbuild in our pipelines.

This PR makes it so that instead of having to pass that property, the projects themselves automatically build with source link when they need to.

### Type of Change
- New feature (non-breaking change which adds functionality)

### Why
This makes it so that local builds of RNW are closer to the the same nuget restore behavior / packages.lock.json file as when they're built in pipelines.

This is part is working toward resolving #12004 and this functionality was extracted from PR #13634.

### What
Instead of requiring us to call msbuild with `/p:EnableSourceLink=true` for every solution (or relying on setting it in `ExperimentalFeatures.props`) we now detect when our library projects are being built within the repo (detecting the `src-win` folder which isn't present in the NPM publish) and enable source link then.

## Screenshots
N/A

## Testing
N/A

## Changelog
Should this change be included in the release notes: _yes_

Automatically set EnableSourceLink for in-repo builds only
jonthysell added a commit that referenced this issue Sep 10, 2024
## Description

Projects "built for Fabric" depend on the `Microsoft.WindowsAppSDK`, while projects "built for Paper" rely on `Microsoft.UI.Xaml`, both of which are mutually exclusive. This leads to a lot of PRs with a spurious flip-flopping of the contents of nuget package lock files, depending on which flavor the author happened to build against.

This PR enables a new separate fabric lock file for projects and also updates all of the lock files.

### Type of Change
- New feature (non-breaking change which adds functionality)

### Why
This makes it so that local builds of RNW are closer to the the same nuget restore behavior / packages.lock.json file as when they're built in pipelines, namely to reduce the number of spurious lock file changes we see in PRs.

This is part is working toward resolving #12004 and this functionality was extracted from PR #13634.

### What
For projects building within our repo that can be built as either Paper or Fabric variants, we now maintain a separate "fabric" nuget lock file: `packages.fabric.lock.json` alongside the normal `packages.lock.json` file (which can be either "paper" or "fabric" depending on what the project normally builds with).

## Screenshots
N/A

## Testing
N/A

## Changelog
Should this change be included in the release notes: _yes_

Create separate NuGet restore lock files for Fabric builds
@chiaramooney chiaramooney added the Workstream: ES Compliance SFI Provide regular ES infrastructure and ensur RNW meets internal security and compliance requirements. label Oct 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: Compliance bug security Pull requests that address a security vulnerability Workstream: ES Compliance SFI Provide regular ES infrastructure and ensur RNW meets internal security and compliance requirements.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants