Fixes for upstream

[alex-ilin]

Some patches for you to consider. Mostly typos in the docs and minor code cleanup, but also there was a resource leak plugged in the VM.

[bjourne]

Here you could go "all the way" and also replace all calls to open-existing with open-r/w.

[alex-ilin]

Interesting idea, but wouldn't that break user code? Or do we not care about that?

[bjourne]

Not in this case because open-existing is not intended for user code.

[bjourne]

Good job. Looks perfectly fine to me. I assume you know to run "io" test or even test-all after big changes?

[alex-ilin]

@bjourne: well, I don't consider any of these changes "big", but you are right, some testing is in order.

Unfortunately, Factor crashes when I run test-all, and I never had the time to babysit it and track down the reasons. I just leave it there for a few hours, and when I return it's gone without a trace. How long does test-all take anyway?

I'll do the "io" test run and report back.

[bjourne]

test-all takes about an hour. Interesting that it crashes, it shouldn't!

[alex-ilin]

I have ran "io" test and got some errors. Some are due to TLS not being supported on Windows, but there was one that might be worth fixing. I'll get to it later.

[alex-ilin]

Here is the test that fails: { { t } [ file-systems [ file-system-info-tuple? ] all? ] }

For some reason one of the volumes triggers T{ windows-error f 2 "The system cannot find the file specified." } in io.files.info.windows:volume>paths when it calls GetVolumePathNamesForVolumeName. Is it OK if I add code to volume>paths to suppress that error?

If yes, then does this code look like the right way to do it?

[
    dup { [ windows-error? ] [ n>> ERROR_FILE_NOT_FOUND = ] } 1&&
    [ drop { } clone ] [ rethrow ] if
] recover

I noticed that in the next word of the same vocab we have a much less specific code to deal with an error:

! Can error with T{ windows-error f 21 "The device is not ready." }
! if there is a D: that is not ready, for instance. Ignore these drives.
M: windows file-systems ( -- array )
    find-volumes [ volume>paths ] map concat [
        [ (file-system-info) ] [ 2drop f ] recover
    ] map sift ;

Should I use the specific error suppression, or just suppress anything and think that there is no volume to give info about anyway?

[alex-ilin]

Here's the code in question, without the error handling I'm about to add:

! Windows may return a volume which looks up to path ""
! For now, treat it like there is not a volume here
: volume>paths ( string -- array )
    [
        names-buf-length
        [ ushort malloc-array &free ] keep
        0 uint <ref>
        [ GetVolumePathNamesForVolumeName win32-error=0/f ] 3keep nip
        uint deref head but-last-slice
        { 0 } split-slice harvest
        [ { } ] [ [ { 0 } append alien>native-string ] map ] if-empty
    ] with-destructors ;

[mrjbq7]

Do you know what string is being passed to it when the error is produced?

[alex-ilin]

@mrjbq7: "Do you know what string is being passed to it when the error is produced?"

Yes, it looks like a valid string to me. Second to last in the array below:

{
    "\\\\?\\Volume{b995ea73-5c69-449d-ae48-b54c846fc86d}\\"
    "\\\\?\\Volume{323544e4-eaee-42bf-b183-1ed8d1294233}\\"
    "\\\\?\\Volume{1e2cbb8a-582f-493e-941e-4413763631ff}\\"
    "\\\\?\\Volume{7fbc23f5-f201-11e5-825e-f0761cd89933}\\"
    "\\\\?\\Volume{1c5a8ef4-340a-430a-bf2f-fe8dfe5e6884}\\"
    "\\\\?\\Volume{d8dde5c9-e316-45d6-b291-bc7f4ceb6cb6}\\"
    "\\\\?\\Volume{ec97f9ae-f4b0-11e5-8279-f0761cd89933}\\"
    "\\\\?\\Volume{ec97fa17-f4b0-11e5-8279-f0761cd89933}\\"
}

[mrjbq7]

I wonder what it is about that path that's invalid - before we just hide errors maybe there is some kind of factor bug with your setup.

[alex-ilin]

@mrjbq7: "I wonder what it is about that path that's invalid - before we just hide errors maybe there is some kind of factor bug with your setup."

I have no idea what's wrong with it. I have 8 items in the list, three of them return valid disk prefixes (b:, c:\ and d:), others return empty string and one errors out. I don't have any more disk letters and/or unmounted CDs.

[alex-ilin]

In volume>paths code quoted above there is this line: [ { } ] [ [ { 0 } append alien>native-string ] map ] if-empty.

Shouldn't there be clone after { }?

[mrjbq7]

The concern is literal empty array might be shared between multiple callers? I suppose if the underlying memory is resized larger an items inserted into the array that could be an issue but not in practical terms I think.

[jonenst]

It depends if you want to guard against the calling code mutating the
returned array or not. "grep '[ { } ]' -r ~/factor" seems to show that
this idiom is already used in several places in the codebase, so I think
it's ok.

Jon

On Thu, Jun 16, 2016 at 8:33 PM, Alexander Iljin notifications@github.com
wrote:

In volume>paths code quoted above there is this line: [ { } ] [ [ { 0 }
append alien>native-string ] map ] if-empty.

Shouldn't there be clone after { }?

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#1655 (comment), or mute
the thread
https://github.com/notifications/unsubscribe/AAFceOVM-M73C5_vTgRyqbrbAB5ijTdyks5qMZcMgaJpZM4I2xhc
.

[alex-ilin]

@jonenst: "this idiom is already used in several places in the codebase, so I think it's ok."

Most of those places are the true quotation of if-empty.

What if all of those places are wrong and need to be fixed?

[alex-ilin]

Other than that (and some TLS fails) the PR passes the "io" test.

[mrjbq7]

I won't be able to review until I get back from my trip early next week. If someone else wants to review and merge before then or I will later.

The tests should all pass generally, our build bots only make new binaries when the tests pass so if you are having test failures then your system is exposing something our build bots are not (or the build
bots are also seeing them).

Looks like they are clean builds no test failures on Windows right now. Maybe this is a good opportunity to see what your OS and architecture is and maybe we can reproduce your failures.

http://builds.factorcode.org/dashboard

[bjourne]

{ } doesn't have to be cloned as empty arrays are immutable. If you download the ssl dlls from http://downloads.factorcode.org/dlls/, the tls errors should go away but they are likely unrelated to the pr anyhow.

[alex-ilin]

@bjourne: I did download the DLL. I even updated the Wiki page on Requirements.

Still, "io.sockets.secure" help begs to differ: At present, this vocabulary only works on Unix, and not on Windows.

[mrjbq7]

Looks like that's a bad help text because it was implemented to work on Windows.

[mrjbq7]

P.S. @alexiljin, thanks for all the debugging fixes and new features!

[alex-ilin]

@mrjbq7: "thanks for all the debugging fixes and new features!"

I really appreciate you saying that! It makes me feel welcome.

@bjourne: you were right, after downloading libssl-38.dll all the TLS tests passed! Previously I assumed I got all the libs I need. Initially some of the tests failed with a message about missing "libcrypto-37.dll", which I downloaded. After that, further tests failed with a message about not having the disposed>> method defined for the f class. And the no-tls-supported error. No mention of another DLL missing. Looking for "no-tls-supported" in help I found that there is no TLS on Windows.

Very misleading, all this. : )

@erg: I've added some io.sockets.secure-docs updates to this PR, please review.

If you feel that the fix to volume>paths is not needed, let's discuss this some more. For me it fixes the test suite.

[mrjbq7]

We have been thinking of shipping a few "required" DLLs with Factor binaries on Windows. Things like SSL, SQLite, etc. basic useful stuff because it's hard to install usually.

[bjourne]

@mrjbq7, @erg anything holding this up from merging?

[mrjbq7]

I was going to try and look tomorrow. Will let you know.

[mrjbq7]

I merged these, including the volume>paths change (which I'd love to understand more -- why is there a volume listed that yields a file not found).

Thanks @alexiljin!