Skip to content

Throw DirectoryNotFoundException on Image.Save with bad directory #34998

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
safern merged 9 commits into from
Apr 17, 2020
Merged

Throw DirectoryNotFoundException on Image.Save with bad directory #34998

safern merged 9 commits into from
Apr 17, 2020

Conversation

@Macromullet
Copy link
Contributor

@Macromullet Macromullet commented Apr 15, 2020

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.

@Macromullet
Provide better exception when saving to non-existent directory
ac525dd 
When saving an image to a non-existent directory, throw a DirectoryNotFoundException
vs a System.Runtime.InteropServices.ExternalException.

Fix #31367
@Macromullet
Change exception message to be more intuitive
c5c2823 
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
Copy link

ghost commented Apr 15, 2020

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

@dnfclas
Copy link

dnfclas commented Apr 15, 2020 •
edited
Loading

CLA assistant check
All CLA requirements met.

@Macromullet
Fix test on unix by using built path.
6ac01e8 
We were using a hardcoded path with backslashes. That didn't match the built path for the Assert.
@Macromullet
Make Test conditional as it requires GDI+
311a3e1
@Macromullet
Put directory check closer to where it's used to ensure that we
05e4978 
capture it from all Save variations.
@Macromullet
Remove trailing whitespace
2ffcd1d
@Macromullet
Copy link
Contributor Author

Macromullet commented Apr 16, 2020

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
Member

danmoseley commented Apr 16, 2020

@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
Contributor Author

Macromullet commented Apr 16, 2020

Thanks @danmosemsft !

safern
safern reviewed Apr 16, 2020
View reviewed changes
@Macromullet
Change CheckIfDirectoryExists to ThrowIfDirectoryDoesntExist
35793e4 
to match naming conventions for throw-style methods.

Fix #31367
safern
safern reviewed Apr 16, 2020
View reviewed changes
@Macromullet
Copy link
Contributor Author

Macromullet commented Apr 16, 2020 •
edited
Loading

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
Copy link
Member

safern commented Apr 16, 2020

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
Contributor Author

Macromullet commented Apr 16, 2020

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
Member

danmoseley commented Apr 17, 2020

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

@Macromullet
Copy link
Contributor Author

Macromullet commented Apr 17, 2020

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
Copy link
Member

safern commented Apr 17, 2020

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
Contributor Author

Macromullet commented Apr 17, 2020

Looks like we are good to go now. Thanks!

@safern safern merged commit 8981ad7 into dotnet:master Apr 17, 2020
@safern safern mentioned this pull request Apr 17, 2020
Closed
@safern
Copy link
Member

safern commented Apr 17, 2020

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.

Reviewers

@safern safern safern approved these changes

Assignees

No one assigned

Labels

area-System.Drawing

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@Macromullet @dnfclas @danmoseley @safern @Dotnet-GitSync-Bot