Clean up make build output

RESOLVED FIXED in 1.0

Status

L20n
JS Library
P1
normal
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: gandalf, Assigned: stas)

Tracking

unspecified
x86
Mac OS X

Details

Attachments

(1 attachment)

(Reporter)

Description

5 years ago
make build output currently is very scary to any newcomer and super noisy for us.

http://pastebin.mozilla.org/2380325
(Assignee)

Comment 1

5 years ago
Created attachment 746339 [details] [diff] [review]
Silence "Failed to find module: amdefine"

The patch removes the "Failed to find module: amdefine" errors by defining a dummy amdefine module and then removing it from the build.  It's a work-around until a fix lands in DryIce:  https://github.com/mozilla/dryice/issues/17

The output now looks like this:

Creating dist/l20n.js
- l20n/html.js has define(...) with non-array parameter. Ignoring requirement.
- l20n.js has define(...) with non-array parameter. Ignoring requirement.
- l20n/context.js has define(...) with non-array parameter. Ignoring requirement.
- l20n/events.js has define(...) with non-array parameter. Ignoring requirement.
- l20n/parser.js has define(...) with non-array parameter. Ignoring requirement.
- l20n/compiler.js has define(...) with non-array parameter. Ignoring requirement.
- l20n/promise.js has define(...) with non-array parameter. Ignoring requirement.
- l20n/retranslation.js has define(...) with non-array parameter. Ignoring requirement.
- Found several matches for l20n/platform/globals.js (ignoring 2nd)
  - /home/stas/moz/l20n/js/lib/client/l20n/platform/globals.js
  - /home/stas/moz/l20n/js/lib/l20n/platform/globals.js
- l20n/platform/globals.js has define(...) with non-array parameter. Ignoring requirement.
- Found several matches for l20n/platform/io.js (ignoring 2nd)
  - /home/stas/moz/l20n/js/lib/client/l20n/platform/io.js
  - /home/stas/moz/l20n/js/lib/l20n/platform/io.js
- l20n/platform/io.js has define(...) with non-array parameter. Ignoring requirement.
- l20n/platform/intl.js has define(...) with non-array parameter. Ignoring requirement.

I like having "Found several matches for l20n/platform/globals.js " in there, but I'd still like to get rid of the "Ignoring requirement." errors.  It looks like this was intended by the author of DryIce to be helpful, but it clearly doesn't work well with the Simple CommonJS Wrapper, which DryIce supports well.  I might try to submit a pull request to make this output optional if I find a moment, but it's not critical.
Assignee: nobody → stas
Attachment #746339 - Flags: review?(gandalf)
(Reporter)

Updated

5 years ago
Attachment #746339 - Flags: review?(gandalf) → review+
(Reporter)

Comment 2

5 years ago
I'd like to remove the Ignoring requirement and for the found several matches - why do we need it? Why can't Makefile point to the right one?

As of now it seems like the AMD is confused about the multiple matches which is a non-optimal state of things
(Assignee)

Comment 3

5 years ago
(In reply to Zbigniew Braniecki [:gandalf] from comment #2)
> I'd like to remove the Ignoring requirement and for the found several
> matches - why do we need it? Why can't Makefile point to the right one?

I'm using DryIce's createCommonJS helper which follows require() calls and names anonymous modules automatically based on the file name.  This way I don't have to explicitly list all files in the Makefile.

Admittedly, it doesn't cost us anything to do so.  I'll experiment tomorrow.
(Assignee)

Comment 4

5 years ago
Attachment #746339 [details] [diff] landed in https://github.com/l20n/l20n.js/commit/33e0a304a976dda9c32e5411c268843343402c36

I'm leaving the bug open to try to fix things mentioned in comment 2.

I submitted https://github.com/jrburke/amdefine/pull/15 which is related.
(Assignee)

Updated

5 years ago
Priority: -- → P4
Target Milestone: --- → 1.0
(Assignee)

Comment 5

5 years ago
(In reply to Staś Małolepszy :stas (needinfo along with cc, please) from comment #4)

> I'm leaving the bug open to try to fix things mentioned in comment 2.

I need further changes in amdefine for this to work: https://github.com/jrburke/amdefine/pull/16
(Assignee)

Comment 6

5 years ago
(In reply to Staś Małolepszy :stas from comment #5)
> I need further changes in amdefine for this to work:
> https://github.com/jrburke/amdefine/pull/16

The pull request was rejected, so I took another approach and made changes to dryice instead:

https://github.com/mozilla/dryice/pull/33
(Assignee)

Updated

5 years ago
Priority: P4 → P1
(Assignee)

Comment 7

5 years ago
Joe merged my changes into his branch:

https://github.com/joewalker/dryice/commit/a0cdb595ec17fe1938ac5d338a602414d0de396c

I landed a change to package.json to use hsi branch for now.  When he upstreams these changes to mozilla/dryice, I'll follow up here.

I also made a small change to how out built files look like:  I removed the deps array from define calls, because we don't need them anyways.

Landed: https://github.com/l20n/l20n.js/commit/bc328b15a07e809a81f4c7bea1749f3a7161f0ca

Here's what the current output of make build looks like:

node build/Makefile.js html

Creating dist/html/l20n.js
- Found several matches for l20n/platform/globals.js (ignoring 2nd)
  - /home/stas/moz/l20n/js/lib/client/l20n/platform/globals.js
  - /home/stas/moz/l20n/js/lib/l20n/platform/globals.js
- Found several matches for l20n/platform/io.js (ignoring 2nd)
  - /home/stas/moz/l20n/js/lib/client/l20n/platform/io.js
  - /home/stas/moz/l20n/js/lib/l20n/platform/io.js

Creating dist/html/l20n.min.js
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.