Skip to content

Conversation

@pjcollins
Copy link
Member

@pjcollins pjcollins commented Nov 30, 2021

I've seen inconsistent XML output from java-source-utils when running on
macOS vs. Windows, or JDK 8 vs. JDK 11. On Windows I've seen extra
carriage returns in CDATA blocks, and when using JDK 9+ we've seen
additional new lines added between XML elements.

An XSL style sheet can be used to improve the formatting consistency of
the XML that is produced. This should prevent us from generating
inconsistent API docs across macOS and Windows.

There is still an outstanding issue concerning CDATA blocks between
JDK 8 and JDK 11 that I have not yet been able to sort out. This issue
has seemingly been "fixed" in JDK 14.

@pjcollins pjcollins marked this pull request as ready for review December 1, 2021 00:33
@pjcollins pjcollins requested a review from jonpryor December 1, 2021 00:33
@pjcollins
Copy link
Member Author

pjcollins commented Dec 1, 2021

@jonpryor there is some overlap here and in #923, I'm not sure which one we want to try to land first, or if we should try to combine them at this point.

String invalidFilePath = "/this/file/does/not/exist";
String osName = System.getProperty("os.name");
if (osName.startsWith("Windows")) {
invalidFilePath = "XYZABC:\\this\\file\\does\\not\\exist";
Copy link
Contributor

Choose a reason for hiding this comment

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

On Windows, wouldn't we want/shouldn't C:\\this\\file\\does\\not\\exist fail? The primary point isn't that the filename itself is invalid -- which would be the case on Windows with XYZABC:\\… -- but rather that a "valid" file path refers a non-existent filesystem entry.

(Plus, on Unix, I'm not sure it's really possible to have a path which itself is invalid. NUL is "valid" {kinda, sorta} in that everything up to the NUL is handled, including newlines!)

Copy link
Member Author

Choose a reason for hiding this comment

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

I believe C:\\this\\file\\does\\not\\exist would not throw a FileNotFoundException when attempting to create a PrintStream, as the mkdirs call makes the path valid. We could change this to use a single non common drive name prefix that doesn't exist (e.g. X or something like that), or we could maybe just ignore this test on Windows.

Copy link
Contributor

Choose a reason for hiding this comment

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

…so now I'm confused, which isn't great: @Test(expected=FileNotFoundException.class) should be asserting that FileNotFoundException is thrown. The fact that this test is passing means that FileNotFoundException is thrown.

No java.io.File APIs document throwing FileNotFoundException, meaning parent.mkdirs() isn't throwing.

This leaves the PrintStream(File) constructor, which is documented as throwing FileNotFoundException:

If the given file object does not denote an existing, writable regular file and a new regular file of that name cannot be created, or if some other error occurs while opening or creating the file

So how's this test work in the first place?

Permissions, apparently; consider:

import java.io.*;

class ps {
    public static void main (String[] args) throws Throwable {
        for (String a : args) {
            File f = new File(a);
            System.out.println(a + " exists? " + f.exists());
            File p = f.getParentFile();
            System.out.println(a + " parent? " + (p != null));
            if (p != null)
                System.out.println("mkdirs on parent? " + p.mkdirs());
            new PrintStream(a);
        }
    }
}

If I run this as:

java ps /this/does/not/exist

I get:

/this/does/not/exist exists? false
/this/does/not/exist parent? true
mkdirs on parent? false
Exception in thread "main" java.io.FileNotFoundException: /this/does/not/exist (No such file or directory)
	at java.base/java.io.FileOutputStream.open0(Native Method)
…

meaning File.mkdirs() (1) doesn't throw, but (2) doesn't do anything, because a non-root user can't create the directory /this.

I imagine the same would be true on Windows.

/me tests…

So on Windows, a non-Admin user can create C:\This. Which is why this test will "fail" on Windows, because File.mkdirs() succeeds. Which is entirely unexpected (to me).

🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

This, on the other hand, fails (as expected):

> mkdir "\Program Files\a"
Access is denied.

So maybe on Windows we need to do:

if (osName.startsWith("Windows")) {
    invalidFilePath = "C:\\Program Files\\does\\not\\exist";
}

Copy link
Member Author

@pjcollins pjcollins Dec 2, 2021

Choose a reason for hiding this comment

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

I think the Microsoft hosted machine pools we use have special permissions that allow the "agent" user to have more access and control. Program Files modification does not appear to be restricted for the Pipelines user account. I'm trying to look for a restricted path but we may ultimately want to skip these two tests on Windows.

Copy link
Member Author

@pjcollins pjcollins Dec 2, 2021

Choose a reason for hiding this comment

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

There are no restrictions in writing to C:\Program Files, C:\Program Files (x86) or C:\Windows on these machines. Rather than continuing to try to seek out a restricted path I've decided to ignore these tests when running on CI on Windows.

@pjcollins pjcollins force-pushed the jsu-xsl-transform branch 2 times, most recently from 945898b to e9975e8 Compare December 2, 2021 18:14
I've seen inconsistent XML output from java-source-utils when running on
macOS vs. Windows, or JDK 8 vs. JDK 11.  On Windows I've seen extra
carriage returns in CDATA blocks, and when using JDK 9+ we've seen
additional new lines added between XML elements.

An XSL style sheet can be used to improve the formatting consistency of
the XML that is produced.  This should prevent us from generating
inconsistent API docs across macOS and Windows.

There is still an outstanding issue concerning CDATA blocks between
JDK 8 and JDK 11 that I have not yet been able to sort out.  This issue
has seemingly been "fixed" in JDK 14.
@jonpryor jonpryor merged commit 7a32bb9 into dotnet:main Dec 3, 2021
@github-actions github-actions bot locked and limited conversation to collaborators Apr 13, 2024
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.

2 participants