1168355 - macBirthDate should be available from the main thread

Status: RESOLVED WONTFIX

(Toolkit Graveyard :: OS.File, defect)

Comment 1

[David Teller [:Yoric] - still alive but not very active]

First thing to do here is to add a test in toolkit/components/osfile/tests/xpcshell/test_creationDate.js to make sure that, on MacOS X, we have a field macBirthDate.

Comment 2

[noitidart]

Hey!! Thanks ok ill try to figure this out :)

Comment 3

[noitidart]

I added this test: https://pastebin.mozilla.org/8834655

add_task(function test_deprecatedCreationDate () {
  let currentDir = yield OS.File.getCurrentDirectory();
  let path = OS.Path.join(currentDir, "test_creationDate.js");

  Services.console.registerListener(consoleListener);
  let result_stat = yield OS.File.stat(path);
  do_check_true(result_stat.macBirthDate != undefined);
});

Comment 4

[noitidart]

whoops left over some crap in there i gotta fix

Comment 5

[David Teller [:Yoric] - still alive but not very active]

Can you turn this into a patch I can review?

See https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/How_to_Submit_a_Patch

Comment 6

[noitidart]

Fixed:

https://pastebin.mozilla.org/8834656

add_task(function test_macBirthDate () {
  if (OS.Constants.Sys.Name == 'Darwin') {
    let result = yield OS.File.stat(path);
    do_check_true(result.macBirthDate != undefined);
  }
});

Comment 7

[noitidart]

Attachment: patch to test if macBirthDate is undefined

Comment 8

[noitidart]

Attachment: 1168355_add-test.patch - undefined path

Oops I had not defined path. I define it in this patch

Comment 9

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8610544 [details] [diff] [review]
patch to test if macBirthDate is undefined

Review of attachment 8610544 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/osfile/tests/xpcshell/test_creationDate.js
@@ +28,5 @@
>    (yield OS.File.stat(path)).creationDate;
>  });
>  
> +add_task(function test_macBirthDate () {
> +  if (OS.Constants.Sys.Name == 'Darwin') {

Nit: We usually write this the other way around.
if (OS.Constants.Sys.Name != 'Darwin') {
  // This is a Mac-only test
  return;
}
let result = ...

@@ +30,5 @@
>  
> +add_task(function test_macBirthDate () {
> +  if (OS.Constants.Sys.Name == 'Darwin') {
> +    let result = yield OS.File.stat(path);
> +    do_check_true(result.macBirthDate != undefined);

For new tests, let's use `Assert.ok` instead of `do_check_true`, as follows:
  `Assert.ok(result.macBirthDate, "The result has a birth date")`

we may also check that `result.macBirthDate` is a date, i.e. that `result.macBirthDate.constructor.name == "Date"`.

Comment 10

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8610787 [details] [diff] [review]
1168355_add-test.patch - undefined path

Review of attachment 8610787 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good with this change.
In the future, since these are small patches, you folding your patches into a single patch?
If you're using mq, the command for folding is `hg qfold`.

Comment 11

[David Teller [:Yoric] - still alive but not very active]

Now, does the test pass on a Mac?

Comment 12

[noitidart]

(In reply to David Rajchenbach-Teller [:Yoric] (use "needinfo") from comment #11)
> Now, does the test pass on a Mac?

Is there any way I can run these tests on moz virtual system? I can't build on my mac as its VM'ed on VirtualBox I tried unsuccessfully over the last few days.

Comment 13

[David Teller [:Yoric] - still alive but not very active]

You'll need a L1 committer agreement, see here: https://www.mozilla.org/hacking/committer/

In the meantime, I checked and the test fails, as expected:
 1:03.59 TEST_STATUS: Thread-70 test_macBirthDate FAIL [test_macBirthDate : 37] false == true
    /Users/david/Documents/Code/mc-testing/obj-x86_64-apple-darwin14.3.0.noindex/_tests/xpcshell/toolkit/components/osfile/tests/xpcshell/test_creationDate.js:test_macBirthDate:37
    self-hosted:next:674
    _run_next_test@/Users/david/Documents/Code/mc-testing/testing/xpcshell/head.js:1440:9
    do_execute_soon/<.run@/Users/david/Documents/Code/mc-testing/testing/xpcshell/head.js:653:9
    _do_main@/Users/david/Documents/Code/mc-testing/testing/xpcshell/head.js:207:5
    _execute_test@/Users/david/Documents/Code/mc-testing/testing/xpcshell/head.js:513:5
    @-e:1:1
 1:03.59 LOG: Thread-70 INFO exiting test

Comment 14

[noitidart]

(In reply to David Rajchenbach-Teller [:Yoric] (use "needinfo") from comment #13)
> You'll need a L1 committer agreement, see here:
> https://www.mozilla.org/hacking/committer/
> 
> In the meantime, I checked and the test fails, as expected:
>  1:03.59 TEST_STATUS: Thread-70 test_macBirthDate FAIL [test_macBirthDate :
> 37] false == true
>    
> /Users/david/Documents/Code/mc-testing/obj-x86_64-apple-darwin14.3.0.noindex/
> _tests/xpcshell/toolkit/components/osfile/tests/xpcshell/test_creationDate.
> js:test_macBirthDate:37
>     self-hosted:next:674
>    
> _run_next_test@/Users/david/Documents/Code/mc-testing/testing/xpcshell/head.
> js:1440:9
>    
> do_execute_soon/<.run@/Users/david/Documents/Code/mc-testing/testing/
> xpcshell/head.js:653:9
>    
> _do_main@/Users/david/Documents/Code/mc-testing/testing/xpcshell/head.js:207:
> 5
>    
> _execute_test@/Users/david/Documents/Code/mc-testing/testing/xpcshell/head.
> js:513:5
>     @-e:1:1
>  1:03.59 LOG: Thread-70 INFO exiting test

Oo ok ill follow that guide to get L1. Is that the next step I should be taking? Or should I start the actual fix?

Comment 15

[noitidart]

ugh i gotta get better at bugzilla, i forgot the needinfo flag, added that with this comment

Comment 16

[David Teller [:Yoric] - still alive but not very active]

If you can start the actual fix, this will be great.

I believe that the error is in OSFileConstants.cpp. Instead of checking for `_DARWIN_FEATURE_64_BIT_INODE` to determine if we should define `OSFILE_OFFSETOF_STAT_ST_BIRTHTIME`, we should check for `HAVE_ST_BIRTHTIME`.

Comment 17

[noitidart]

Attachment: 0001-Bug-1168355-1-Makes-macBirthDate-gettable-2-Makes-ma.patch

(1) Makes macBirthDate gettable (2) Makes macBirthDate available not if it is just a mac, but if it HAVE_ST_BIRTHTIME (3) the test is not just for mac it tests all non-windows system

Comment 18

[noitidart]

Attachment: 0001-Bug-1168355-Improved-the-test.patch

Improves the test, before i was testing if non-windows. but i should check if it has the const.

Comment 19

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8788708 [details] [diff] [review]
0001-Bug-1168355-1-Makes-macBirthDate-gettable-2-Makes-ma.patch

Review of attachment 8788708 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, thanks.

::: dom/system/OSFileConstants.cpp
@@ +689,5 @@
>    { "OSFILE_OFFSETOF_STAT_ST_MTIME", JS::Int32Value(offsetof (struct stat, st_mtime)) },
>    { "OSFILE_OFFSETOF_STAT_ST_CTIME", JS::Int32Value(offsetof (struct stat, st_ctime)) },
>  #endif // defined(HAVE_ST_ATIME)
>  
>    // Several OSes have a birthtime field. For the moment, supporting only Darwin.

Let's remove the second sentence.

Comment 20

[noitidart]

Attachment: 0001-Bug-1168355-1-Makes-macBirthDate-gettable-2-Makes-ma(2).patch

patch commited to remove part of sentence that says "darwin only"

Comment 21

[noitidart]

One thing I didn't understand @Yoric is that I can't find `HAVE_ST_BIRTHTIME` being declared anywhere.
I was asking because after this bug I was hoping to tackle - https://bugzilla.mozilla.org/show_bug.cgi?id=1168356 - and for this I would need `HAVE_CRTIME` for `crtime`, `HAVE_OTIME` for `otime`, and anything other unique fields.

Comment 22

[David Teller [:Yoric] - still alive but not very active]

Theoretically, HAVE_ST_BIRTHTIME should be declared in the system headers, so part of MacOS / BSD / Linux. Unfortunately, I have checked and it's not defined in MacOS, despite the fact that the field is defined. Sigh.

I'm almost sure that there is no such thing as HAVE_CRTIME. On what platform would this be useful?

Comment 23

[noitidart]

Oh I was asking preemptively as when I start working on this bug1168356 I will maybe test filesystem or just test for existence of the crtime or otime fields. And for HFS+ maybe other fields per this stackoverflow topic - http://stackoverflow.com/a/10172234/1828637

Also I'm learning what to do with patches after receiving r+, will get this moving soon. I couldn't test this patch on macos as I did this with janitor.

Comment 24

[noitidart]

@Yoric I did need info to you, but I don't really need a reply, that last reply was just a FYI. In these cases should I still do a need info to you?

Comment 25

[David Teller [:Yoric] - still alive but not very active]

Yeah, you can needinfo me. Just don't be surprised if I don't answer quickly :)

Comment 26

[noitidart]

Thanks I understand. For the FYI I'll understand too if you just clear the needinfo flag without reply :)

Comment 27

[Barret Rennie [:barret] (they/them)]

Mass closure: OSFIle is being replaced with IOUtils and PathUtils. If you think this bug was closed in error, please re-open.