Skip to content

Conversation

@jonathanpeppers
Copy link
Member

Context: dotnet/android#978

When bumping java.interop in xamarin-android, a test failure occurred
in the ClassPath.GetDocletType method. This was added in #188, but
since ANDROID_SDK_PATH is not set during java.interop’s test runs the
test was skipped.

The incoming path can be a directory, so the File.OpenText call
will fail in that case. Added a File.Exists check as a fix.

Context: dotnet/android#978

When bumping java.interop in xamarin-android, a test failure occurred
in the `ClassPath.GetDocletType` method. This was added in dotnet#188, but
since `ANDROID_SDK_PATH` is not set during java.interop’s test runs the
test was skipped.

The incoming `path` can be a directory, so the `File.OpenText` call
will fail in that case. Added a `File.Exists` check as a fix.
@dnfclas
Copy link

dnfclas commented Oct 26, 2017

@jonathanpeppers,
Thanks for having already signed the Contribution License Agreement. Your agreement was validated by .NET Foundation. We will now review your pull request.
Thanks,
.NET Foundation Pull Request Bot

@jonpryor
Copy link
Contributor

jonpryor commented Oct 26, 2017

This PR concerns me, because the entire method is starting to concern me. path isn't modified, yet we have Path.Combine(path, "packages.html"), which suggests that path should be a directory.

Then there's line 243:

using (var reader = File.OpenText (Path.Combine (path, "index.html")))

If path is a file, that will throw; there's no File.Exists() check to ensure that if e.g. path were a file and we read filename/index.html -- an invalid filename -- that we won't throw.

Which makes line 255 really weird and suspect: if path must be a directory, then how did this ever work?

Did it ever work? PR #188 was only merged 22 days ago; maybe that code path hasn't actually been used or tested yet, especially since PR #188 contained no unit tests. (Doh!)

using (var reader = File.OpenText (path)) {
int len = reader.ReadBlock (buf, 0, buf.Length);
rawXML = new string (buf, 0, len);
if (File.Exists (path)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

...Which means this proposed "fix" is Doomed To Fail™: if path must be a directory, then File.Exist(path) will always be false, meaning this code is dead code; it's not executable.

I think we need @Redth to chime in.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, the ClassParse task just passes in DocumentationPaths strings which usually is a directory, but could be a file, as the ApiXmlDocPaths is intended to be a path to an xml file.

I think this fix should work in this case since File.Exists will indeed skip over directories and this code is only hit for files.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a quick test we could add with a file? Looking at the code, I think this part would fail if a file path came in:

using (var reader = File.OpenText (Path.Combine (path, "index.html")))
    reader.ReadBlock (buf, 0, buf.Length);

@jonathanpeppers
Copy link
Member Author

@Redth is going to address in another PR

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants