Closed Bug 1108088 Opened 9 years ago Closed 9 years ago

./build-jsx doesn't work on windows

Categories

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

All
Windows 8
defect
Points:
3

Tracking

(firefox40 fixed)

RESOLVED FIXED
mozilla40
Iteration:
40.3 - 11 May
Tracking Status
firefox40 --- fixed
backlog tech-debt

People

(Reporter: standard8, Assigned: standard8)

Details

(Whiteboard: [tech-debt])

Attachments

(1 file)

      No description provided.
backlog: --- → tech-debt
Priority: -- → P2
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.
Attachment #8594752 - Flags: review?(mdeboer)
Assignee: nobody → standard8
Points: --- → 3
Attachment #8594752 - Flags: review?(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]:
-----------------------------------------------------------------

I'm no Python hero, sorry.
Attachment #8594752 - Flags: review?(mdeboer)
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.
Attachment #8594752 - Flags: review?(dmose) → review+
Iteration: --- → 40.3 - 11 May
Flags: qe-verify-
Flags: firefox-backlog+
https://hg.mozilla.org/mozilla-central/rev/7dbd445d2abf
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
You need to log in before you can comment on or make changes to this bug.