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.

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

fixes #114 - add windows abstraction to handle file linking #136

Closed
wants to merge 1 commit into from

Conversation

swelham
Copy link

@swelham swelham commented Mar 31, 2015

I have added a layer on top of the existing EmberCLI::App class that handles the linking when running on windows.

While the solution isn't pretty (and is a bit of a hack) I have found this to work reliably on windows.

def command(options={})
watch = options[:watch] ? "--watch" : ""

"\"#{ember_path}.cmd\" build #{watch} --environment #{environment} --output-path \"#{dist_path}\" #{log_pipe}"
Copy link
Collaborator

Choose a reason for hiding this comment

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

This can't be right. The code is designed to blow up when ember_path is not an executable file. So, it's not gonna return something sensible that you can just add .cmd to, I think.

Copy link
Author

Choose a reason for hiding this comment

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

The reason I changed it to check exist? is because on windows executable? will always return false here. However I totally forgot that npm will create a .cmd for it which might work.

Copy link

Choose a reason for hiding this comment

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

@swelham is right. Windows doesn't understand hashbang (#!) executables, so npm creates a .cmd wrapper that basically calls node.exe <script>. So if an npm creates a binstub on windows, then it also creates a .cmd file so that it can be used as an executable. Ruby does the same thing, except as batch scripts.

@seanpdoyle
Copy link
Contributor

We're sorry that this issue has sat for so long.

The project is now maintained by thoughtbot, and we're declaring an issue bankruptcy of sorts.

This looks promising, and we're really interested fixing this problem. However, since this project lacks a test suite (and CI in general), we're going to hold off on merging PRs until we've added some coverage.

Thanks for your patience.

@seanpdoyle
Copy link
Contributor

@swelham sorry that this issue has sat for so long!

Luckily, we've restructured the way shell interaction is implemented:

We now have a Command class that is responsible for generating the proper CLI string.

If it's possible, could you list out the differences between generated strings from a *NIX / Windows perspective?

Even better, could you update this PR to introduce a WindowCommand that could be swapped in for Command when the windows platform is detected?

@seanpdoyle
Copy link
Contributor

Unfortunately, I don't have access to a Windows machine to verify this works.

There is a branch that introduces AppVeyor for CI testing:

#291

Would you mind helping to create windows versions of:

  • bin/setup -
  • bin/setup_ember -
    • adds a catchall route to the Ember application
    • copies an image file into the ember application
    • adds the image to a template

Once we have setup parity across platforms, we can start to ensure the Windows version behaves the same as other Operating Systems versions.

@seanpdoyle seanpdoyle deleted the branch tricknotes:master December 20, 2021 15:31
@seanpdoyle seanpdoyle closed this Dec 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants