1319854 - Unused resource://gre/modules/osfile/osfile_{unix,win}_{back,front}.jsm files

Status: RESOLVED FIXED

(Toolkit Graveyard :: OS.File, defect)

Description

[Florian Quèze [:florian]]

The test I'm working on in bug 1316187 reports:

File                                                Unreferenced on:
resource://gre/modules/osfile/osfile_unix_back.jsm  windows
resource://gre/modules/osfile/osfile_unix_front.jsm windows
resource://gre/modules/osfile/osfile_win_back.jsm   linux,mac
resource://gre/modules/osfile/osfile_win_front.jsm  linux,mac

Comment 1

[Florian Quèze [:florian]]

Attachment: Fix

This does the straightforward moz.build change. I do wonder if we still need the ospath.jsm file and couldn't instead package the _win and _unix variants of each of these files in a way that would make them end up at the same url so that we could import them without OS.Constants checks.

Comment 2

[:Gijs (out for now; he/him)]

(In reply to Florian Quèze [:florian] [:flo] from comment #1)
> Created attachment 8813763 [details] [diff] [review]
> Fix
> 
> This does the straightforward moz.build change. I do wonder if we still need
> the ospath.jsm file and couldn't instead package the _win and _unix variants
> of each of these files in a way that would make them end up at the same url
> so that we could import them without OS.Constants checks.

Could do this by using subdirs, I think? So if you had win/foo.jsm and unix/foo.jsm, you could add the path to EXTRA_JS_MODULES.osfile thing, which I'm reasonably sure strips off the extra subdirs?

Comment 3

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

Comment on attachment 8813763 [details] [diff] [review]
Fix

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

Looks good to me.
Note that I'd like to find time to rewrite OS.File in Rust, one of these days :)

Comment 4

[Florian Quèze [:florian]]

(In reply to :Gijs Kruitbosch from comment #2)
> (In reply to Florian Quèze [:florian] [:flo] from comment #1)
> > Created attachment 8813763 [details] [diff] [review]
> > Fix
> > 
> > This does the straightforward moz.build change. I do wonder if we still need
> > the ospath.jsm file and couldn't instead package the _win and _unix variants
> > of each of these files in a way that would make them end up at the same url
> > so that we could import them without OS.Constants checks.
> 
> Could do this by using subdirs, I think? So if you had win/foo.jsm and
> unix/foo.jsm, you could add the path to EXTRA_JS_MODULES.osfile thing, which
> I'm reasonably sure strips off the extra subdirs?

Using subdirs in this way is also what I had in mind. David, would you prefer this?

Comment 5

[Florian Quèze [:florian]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/bdbcf49b3359dbcad12de40de0a25148cdf1804b
Bug 1319854 - Unused resource://gre/modules/osfile/osfile_{unix,win}_{back,front}.jsm files, r=Yoric.

Comment 6

[Florian Quèze [:florian]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d6ec40f470fced56227e005c08ee40a0f5a47b5b
Bug 1319854 - Follow-up to make the test_path.js xpcshell test not depend on files from other platforms, rs=bustage-fix.

Comment 7

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f62ce5db4ee2
Backed out changeset d6ec40f470fc 
https://hg.mozilla.org/integration/mozilla-inbound/rev/670ad168fb70
Backed out changeset bdbcf49b3359 for continued various test failures

Comment 8

[Carsten Book [:Tomcat]]

backed this out in https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=670ad168fb708a7eca83480419f6206833b2566e because of continued test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=39930510&repo=mozilla-inbound

maybe this need a try run to find out what was going wrong

Comment 9

[Florian Quèze [:florian]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7b38319665e3

Comment 10

[Florian Quèze [:florian]]

Attachment: make loader.js use ospath.jsm instead of ospath_unix.jsm

Importing resource://gre/modules/osfile/ospath_unix.jsm on Windows is what caused my patch to be backed out.

Comment 11

[Alexandre Poirot [:ochameau]]

Comment on attachment 8815030 [details] [diff] [review]
make loader.js use ospath.jsm instead of ospath_unix.jsm

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

Unfortunately, SDK loader expects unix paths, with / and not \.
So I imagine it will break with this change.
`join` should return path seperated and we will pass paths with / as seperator to `normalize` and `dirname`...

You could keep just this file, or inline some simplified versions (I'm not sure it depends on very complex implementation?).

Comment 12

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

(In reply to Florian Quèze [:florian] [:flo] from comment #4)
> Using subdirs in this way is also what I had in mind. David, would you
> prefer this?

I have no preference here.

Comment 13

[Florian Quèze [:florian]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4dfb387559eb

Comment 14

[Florian Quèze [:florian]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d03852230a32f2eaba331655823c3c814bdac179
Bug 1319854 - Unused resource://gre/modules/osfile/osfile_{unix,win}_{back,front}.jsm files, r=Yoric.

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d03852230a32