1108088 - ./build-jsx doesn't work on windows

Status: RESOLVED FIXED

(Hello (Loop) :: Client, defect, P2)

Comment 1

[Mark Banner (:standard8)]

Attachment: Get Loop's build-jsx working on Windows without the react version check working (for now). NPOTB DONTBUILD

This has been on my pick-up list for a while, and we had someone else want to use windows for development recently. Given that, I picked this up and worked out a few issues.

The biggest pain point is that it seems very difficult to capture the output of jsx -V in python on windows for some reason. I've tried multiple methods and not found a working one just yet. I'll probably end up asking in #ateam or something, but that's for another day.

What I did manage to get working was the rest of the build-jsx functionality which should make it easier for people on Windows, albeit at the slight risk of their react being out of date. Hopefully we can find a solution for that soon, just not yet.

Mike: if you're not comfortable reviewing this, please pass to Dan or Jared.

Comment 2

[Mike de Boer [:mikedeboer]]

Comment on attachment 8594752 [details] [diff] [review]
Get Loop's build-jsx working on Windows without the react version check working (for now). NPOTB DONTBUILD

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

I'm no Python hero, sorry.

Comment 3

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 8594752 [details] [diff] [review]
Get Loop's build-jsx working on Windows without the react version check working (for now). NPOTB DONTBUILD

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

r=dmose.  In addition to the bits I've suggested considering inline, it'd be nice to put docstrings in the two new functions you've added.

::: browser/components/loop/build-jsx
@@ +42,2 @@
>  # search for react-tools install
> +if sys.platform == 'win32':

It might be worth putting this into it's own method.  It's a pretty big chunk to have at the top-level.

@@ +42,5 @@
>  # search for react-tools install
> +if sys.platform == 'win32':
> +    # spawn.find_executable assumes a '.exe' extension on windows
> +    # something which jsx doesn't have...
> +    def find_excutable_no_extension(fileName):

It'd be nice to use a docstring here instead of just a comment, so that this shows up when working in the interpreter/debugger.

Comment 4

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/7dbd445d2abf

Comment 5

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/7dbd445d2abf