Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Improving Windows support #742

Merged
merged 7 commits into from
Mar 30, 2021
Merged

Improving Windows support #742

merged 7 commits into from
Mar 30, 2021

Conversation

thoov
Copy link
Collaborator

@thoov thoov commented Mar 26, 2021 •
edited
Loading

Fixes: #681 & #676, along with another issue discovered after those two were resolved.

Ember-cli's test suite caught an issue with Windows support when trying to enable Embroider by default (ember-cli/ember-cli#9495). After this change I was able to successfully build with a new Ember app using Embroider on Windows.

We should land: #739 first to verify that the builds fully pass under windows.

@thoov
Copy link
Collaborator Author

thoov commented Mar 26, 2021

CC: @rwjblue / @stefanpenner / @ef4

@thoov thoov mentioned this pull request Mar 26, 2021
Merged
@lifeart
Copy link
Collaborator

lifeart commented Mar 26, 2021

Looks like all issues described in #678 fixed, @thoov thank you!

@thoov thoov added the bug Something isn't working label Mar 26, 2021
@RobbieTheWagner
Copy link

It's awesome to see this getting resolved. Thanks @thoov!

@rwjblue rwjblue linked an issue Mar 26, 2021 that may be closed by this pull request
Error on Windows #676
Closed
@rwjblue rwjblue mentioned this pull request Mar 26, 2021
Closed
@thoov
Copy link
Collaborator Author

thoov commented Mar 26, 2021

Based on #744

Failing for infra related reasons still being worked on:

@embroider/util ember-canary windows
@embroider/util ember-beta windows
@embroider/util classic windows
@embroider/util ember windows
@embroider/router classic windows
static-app custom-relative-root windows
static-app custom-root windows
static-app ember-classic windows
macro-tests ember-classic windows
macro-sample-addon classic: windows
fastboot-app fastboot-classic windows
fastboot-app fastboot-production windows
fastboot-app ember-classic windows
engines-host-app fastboot-classic windows
engines-host-app ember-classic windows

Failing for windows reasons that might still be infra related:

fastboot-app fastboot windows
fastboot-app ember-production windows
fastboot-app ember windows
engines-host-app fastboot windows

Failing for newly discovered Windows reasons:

node windows
engines-host-app ember windows

@rwjblue rwjblue requested a review from ef4 March 29, 2021 14:44
@thoov
Improving windows support
4625b83 
Fixes: embroider-build#681 & embroider-build#676

Ember-cli's test suite caught an issue with windows support when
trying to enable embroider by default
(ember-cli/ember-cli#9495). After this change
I was able to successfully do a build with a new Ember app.
@thoov
better absolute logic for windows drives
0838550
@thoov
Use jsStringEscape for better back slash handling
2ac6e44
stefanpenner
stefanpenner requested changes Mar 29, 2021
View reviewed changes
packages/core/src/template-compiler-common.ts Outdated Show resolved Hide resolved
packages/core/src/template-compiler-common.ts Outdated Show resolved Hide resolved
stefanpenner
stefanpenner reviewed Mar 29, 2021
View reviewed changes
Copy link
Collaborator

@stefanpenner stefanpenner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given that this project will be shipping as part of ember-cli, it would be appropriate for it to have windows CI running. Should that be part of this pr or an upcoming PR (if so, is this work planned and do we have an ETA?)?

packages/core/src/template-compiler-common.ts Outdated Show resolved Hide resolved
packages/core/src/template-compiler-common.ts Outdated Show resolved Hide resolved
@thoov
Copy link
Collaborator Author

thoov commented Mar 29, 2021

@stefanpenner

Given that this project will be shipping as part of ember-cli, it would be appropriate for it to have windows CI running. Should that be part of this pr or an upcoming PR (if so, is this work planned and do we have an ETA?)?

@rwjblue is adding that here: #739

@ef4
Copy link
Contributor

ef4 commented Mar 30, 2021 •
edited
Loading

I merged in master, it might have introduced a couple more places that need path handling adjustments, let's see what CI thinks. Feel free to merge once green.

@ef4 ef4 merged commit 9660ab9 into embroider-build:master Mar 30, 2021
@thoov thoov deleted the windows-support branch March 30, 2021 21:23
thoov added a commit to thoov/embroider that referenced this pull request Jun 23, 2021
@thoov
Template compiler plugin not removed due to bad path comparison on
6b9b60f 
Windows

Same issue that was fixed in: embroider-build#742
@thoov thoov mentioned this pull request Jun 23, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stefanpenner stefanpenner stefanpenner requested changes

@rwjblue rwjblue rwjblue approved these changes

@lifeart lifeart lifeart approved these changes

@RobbieTheWagner RobbieTheWagner RobbieTheWagner approved these changes

@ef4 ef4 Awaiting requested review from ef4

Assignees
No one assigned
Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Ember serve fails on windows with embroider 0.32+ Error on Windows
6 participants
@thoov @lifeart @RobbieTheWagner @ef4 @stefanpenner @rwjblue