Skip to content

Throw DirectoryNotFoundException on Image.Save with bad directory - #34998

Merged
safern merged 9 commits into
dotnet:masterfrom
Macromullet:master
Apr 17, 2020
Merged

Throw DirectoryNotFoundException on Image.Save with bad directory#34998
safern merged 9 commits into
dotnet:masterfrom
Macromullet:master

Conversation

@Macromullet

Copy link
Copy Markdown
Contributor

This provides a more meaningful exception when saving an Image to a directory that does not exist. Addresses #31367

This does change behavior slightly per https://github.com/dotnet/runtime/blob/master/docs/coding-guidelines/breaking-changes.md#bucket-2-reasonable-grey-area, as previously, the operation would throw a System.Runtime.InteropServices.ExternalException.

When saving an image to a non-existent directory, throw a DirectoryNotFoundException
vs a System.Runtime.InteropServices.ExternalException.

Fix #31367
Use: The directory {0} of the filename {1} does not exist.
instead of: The target directory {0} of the targetPath {1} does not exist.

Fix #31367
@ghost

ghost commented Apr 15, 2020

Copy link
Copy Markdown

Tagging subscribers to this area: @safern
Notify danmosemsft if you want to be subscribed.

@dnfclas

dnfclas commented Apr 15, 2020

Copy link
Copy Markdown

CLA assistant check
All CLA requirements met.

We were using a hardcoded path with backslashes. That didn't match the built path for the Assert.
@Macromullet

Copy link
Copy Markdown
Contributor Author

This is working in .net core, but failing net472 tests. Is this because in that mode it's acting as a facade and just redirecting to the .net framework implementation? If so, I don't see a way of making the tests pass across the board, without understanding the compatibility change, as the behavior in .net 472 was to throw the GDI+ exception as well.

@danmoseley

Copy link
Copy Markdown
Contributor

@Macromullet you're exactly right. Some libraries' tests (like this one) test on .NET Framework as well even though we don't update add features to them on .NET Framework any more. This is just to catch inadvertent compat breaks. In this case it's intentional. Please fix it by adding [SkipOnTargetFramework(TargetFrameworkMonikers.NetFramework, ".NET Framework throws ExternalException")] or similar to those test(s).

@Macromullet

Copy link
Copy Markdown
Contributor Author

Thanks @danmosemsft !

Comment thread src/libraries/System.Drawing.Common/src/System/Drawing/Image.cs Outdated
to match naming conventions for throw-style methods.

Fix #31367
Comment thread src/libraries/System.Drawing.Common/tests/ImageTests.cs Outdated
@Macromullet

Macromullet commented Apr 16, 2020

Copy link
Copy Markdown
Contributor Author

The failing check (runtime-live-build (Build OSX x64 release Runtime_Debug)) seems to be a transient issue related to nuget downloads. Is there a way to rerun the pipeline?

.dotnet/sdk/5.0.100-preview.4.20202.8/NuGet.targets(128,5): error : (NETCORE_ENGINEERING_TELEMETRY=Restore) The feed 'nuget.org [https://api.nuget.org/v3/index.json]' lists package 'Microsoft.NETCore.DotNetHostResolver.2.1.0' but multiple attempts to download the nupkg have failed. The feed is either invalid or required packages were removed while the current operation was in progress. Verify the package exists on the feed and try again.

@safern

safern commented Apr 16, 2020

Copy link
Copy Markdown
Member

Yeah. I've been seen that across multiple builds, it seems like nuget.org is having some trouble (probably outage) that is hitting us. We can retry the build when the others finish. Anyway you had a typo which will cause you to update the PR and the builds will restart.

@Macromullet

Copy link
Copy Markdown
Contributor Author

The pipeline seems to stall out from time to time. Is this normal? Is it a prioritization issue via the number of available agents?

@danmoseley

Copy link
Copy Markdown
Contributor

@Macromullet yes there are some queues that have been a bit low on machines recently.

@Macromullet

Copy link
Copy Markdown
Contributor Author

So the checks failed here:
Starting: System.IO.FileSystem.Tests (parallel test collections = on, max threads = 2)
System.IO.Tests.DirectoryInfo_Create.RootPath_AppContainer [SKIP]
Condition(s) not met: "IsInAppContainer"
System.IO.Tests.File_GetSetTimes.TimesIncludeMillisecondPart_HFS [SKIP]
Condition(s) not met: "isHFS"

That's outside the scope of this change. I'm unsure how to proceed since I don't control that body of code.

@safern

safern commented Apr 17, 2020

Copy link
Copy Markdown
Member

That completely seems like an unrelated error. Will retry the build, but will save the links to investigate and open an issue accordingly.

@Macromullet

Copy link
Copy Markdown
Contributor Author

Looks like we are good to go now. Thanks!

@safern
safern merged commit 8981ad7 into dotnet:master Apr 17, 2020
@safern

safern commented Apr 17, 2020

Copy link
Copy Markdown
Member

Thanks, @Macromullet for the contribution 😄! For the test failure I opened #35117

@ghost ghost locked as resolved and limited conversation to collaborators Dec 9, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants