macBirthDate should be available from the main thread

NEW
Assigned to

Status

()

Toolkit
OS.File
3 years ago
2 years ago

People

(Reporter: Yoric, Assigned: noitidart)

Tracking

(Blocks: 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

Comment hidden (empty)
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.
Flags: needinfo?(noitidart)
(Assignee)

Comment 2

3 years ago
Hey!! Thanks ok ill try to figure this out :)
Flags: needinfo?(noitidart)
(Assignee)

Comment 3

3 years ago
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);
});
(Assignee)

Comment 4

3 years ago
whoops left over some crap in there i gotta fix
(Assignee)

Comment 6

3 years ago
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);
  }
});
(Assignee)

Comment 7

3 years ago
Created attachment 8610544 [details] [diff] [review]
patch to test if macBirthDate is undefined
Attachment #8610544 - Flags: feedback?(dteller)
(Assignee)

Comment 8

3 years ago
Created attachment 8610787 [details] [diff] [review]
1168355_add-test.patch - undefined path

Oops I had not defined path. I define it in this patch
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"`.
Attachment #8610544 - Flags: feedback?(dteller) → feedback+
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`.
Attachment #8610787 - Flags: feedback+
Now, does the test pass on a Mac?
Assignee: nobody → noitidart
Flags: needinfo?(noitidart)
(Assignee)

Comment 12

3 years ago
(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.
Flags: needinfo?(noitidart) → needinfo?(dteller)
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
Flags: needinfo?(dteller) → needinfo?(noitidart)
(Assignee)

Comment 14

3 years ago
(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?
Flags: needinfo?(noitidart)
(Assignee)

Comment 15

3 years ago
ugh i gotta get better at bugzilla, i forgot the needinfo flag, added that with this comment
Flags: needinfo?(dteller)
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`.
Flags: needinfo?(dteller) → needinfo?(noitidart)
(Assignee)

Comment 17

2 years ago
Created attachment 8788708 [details] [diff] [review]
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
Attachment #8610544 - Attachment is obsolete: true
Attachment #8610787 - Attachment is obsolete: true
Flags: needinfo?(noitidart) → needinfo?(dteller)
Attachment #8788708 - Flags: review?(dteller)
(Assignee)

Comment 18

2 years ago
Created attachment 8788710 [details] [diff] [review]
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 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.
Attachment #8788708 - Flags: review?(dteller) → review+
(Assignee)

Comment 20

2 years ago
Created attachment 8788972 [details] [diff] [review]
0001-Bug-1168355-1-Makes-macBirthDate-gettable-2-Makes-ma(2).patch

patch commited to remove part of sentence that says "darwin only"
Attachment #8788708 - Attachment is obsolete: true
Attachment #8788710 - Attachment is obsolete: true
Flags: needinfo?(dteller)
(Assignee)

Comment 21

2 years ago
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.
Flags: needinfo?(dteller)
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?
Flags: needinfo?(dteller) → needinfo?(noitidart)
(Assignee)

Comment 23

2 years ago
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.
Flags: needinfo?(noitidart) → needinfo?(dteller)
(Assignee)

Comment 24

2 years ago
@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?
Yeah, you can needinfo me. Just don't be surprised if I don't answer quickly :)
Flags: needinfo?(dteller)
(Assignee)

Comment 26

2 years ago
Thanks I understand. For the FYI I'll understand too if you just clear the needinfo flag without reply :)
You need to log in before you can comment on or make changes to this bug.