Skip to content

Move NoWarn=NU1903 to the Wix PackageReference #11911

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

Jump to bottom
Merged
merged 1 commit into from
Sep 13, 2024
Merged

Move NoWarn=NU1903 to the Wix PackageReference #11911

merged 1 commit into from
Sep 13, 2024

Conversation

ericstj
Copy link
Contributor

@ericstj ericstj commented Sep 12, 2024

#11574 @RussKie

Proposed changes

  • Instead of suppressing warning globally for the repo, suppress it on the single package reference where its needed. This lets GitExtensions still see warnings for other vulnerable packages.

Test methodology

  • Restore before change - works
  • Remove NoWarn - fails with NU1903
  • Add NoWarn to package reference - works again

Test environment(s)

Windows, dotnet 9.0 preview 7 SDK

Merge strategy

  • Squash merge (maintainer to decide merge message, PR submitter should cleanup commits/messages at PR approval).

I agree that the maintainer squash merge this PR (if the commit message is clear).

✒️ I contribute this code under The Developer Certificate of Origin.

@ericstj ericstj mentioned this pull request Sep 12, 2024
Open
@pmiossec
Copy link
Member

👍 I don't even known it was possible.
But maybe we should try to update win to v3.14.1 first ? https://wixtoolset.org/news/2024/02/06/wix-v4.0.4-available/

@RussKie
Copy link
Member

RussKie commented Sep 13, 2024

Thank you @ericstj

@RussKie RussKie merged commit 90f9188 into gitextensions:master Sep 13, 2024
3 checks passed
@RussKie
Copy link
Member

RussKie commented Sep 13, 2024

👍 I don't even known it was possible. But maybe we should try to update win to v3.14.1 first ? wixtoolset.org/news/2024/02/06/wix-v4.0.4-available

Let's try to update to 3.14.
Last time I tried migrating to v4 it turned out to be a complete disaster...

pmiossec added a commit to pmiossec/gitextensions that referenced this pull request Sep 13, 2024
@pmiossec
chore: Update wix to v3.14.1 (security update)
c4a19fa 
No more need to ignore warning NU1903 raised by Wix package <= v3.14.0
(done in gitextensions#11574 and gitextensions#11911 )

https://wixtoolset.org/news/2024/02/06/wix-v4.0.4-available/
pmiossec added a commit to pmiossec/gitextensions that referenced this pull request Sep 13, 2024
@pmiossec
chore: Update wix to v3.14.1 (security update)
853f870 
No more need to ignore warning NU1903 raised by Wix package <= v3.14.0
(better fix for gitextensions#11574 than ones done in gitextensions#11575 and gitextensions#11911 )

https://wixtoolset.org/news/2024/02/06/wix-v4.0.4-available/
@pmiossec pmiossec mentioned this pull request Sep 13, 2024
Merged
@ericstj
Copy link
Contributor Author

ericstj commented Sep 13, 2024

👍 I don't even known it was possible. But maybe we should try to update win to v3.14.1 first ? https://wixtoolset.org/news/2024/02/06/wix-v4.0.4-available/

I believe it flows transitively too. I try to never apply warning suppressions globally if I can use a pragma or some form of local suppression like this. Here's the docs: https://learn.microsoft.com/en-us/visualstudio/ide/how-to-suppress-compiler-warnings?view=vs-2022#suppress-warnings-for-nuget-packages

pmiossec added a commit that referenced this pull request Sep 17, 2024
@pmiossec
chore: Update wix to v3.14.1 (security update)
2f44802 
No more need to ignore warning NU1903 raised by Wix package <= v3.14.0
(better fix for #11574 than ones done in #11575 and #11911 )

https://wixtoolset.org/news/2024/02/06/wix-v4.0.4-available/
@RussKie RussKie added this to the 5.1 milestone Nov 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@RussKie RussKie RussKie approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
5.1
Development

Successfully merging this pull request may close these issues.
3 participants
@ericstj @pmiossec @RussKie