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

Allow look ups to succeed even if the build was done on Windows #1205

Closed
wants to merge 3 commits into from
Closed

Allow look ups to succeed even if the build was done on Windows #1205

wants to merge 3 commits into from

Conversation

cshupp1
Copy link
Contributor

@cshupp1 cshupp1 commented Jan 22, 2018 •
edited
Loading

This is a fix for:

#1174

Originally, when I wrote up the defect I assumed the gem was writing the manifest.json file. By holding a lock to manifest.json the stack trace reveals:

Error: EBUSY: resource busy or locked, open 'C:\work\VSO\vso\public\packs\manifest.json'
    at Object.fs.openSync (fs.js:646:18)
    at Object.fs.writeFileSync (fs.js:1291:33)
    at Object.outputFileSync (C:\work\VSO\vso\node_modules\fs-extra\lib\output\index.js:26:29)
    at ManifestPlugin.<anonymous> (C:\work\VSO\vso\node_modules\webpack-manifest-plugin\lib\plugin.js:158:11)
    at Compiler.applyPluginsAsyncSeries (C:\work\VSO\vso\node_modules\tapable\lib\Tapable.js:206:13)
    at Compiler.emitAssets (C:\work\VSO\vso\node_modules\webpack\lib\Compiler.js:354:8)
    at onCompiled (C:\work\VSO\vso\node_modules\webpack\lib\Compiler.js:58:19)
    at applyPluginsAsync.err (C:\work\VSO\vso\node_modules\webpack\lib\Compiler.js:510:14)
    at next (C:\work\VSO\vso\node_modules\tapable\lib\Tapable.js:202:11)
    at Compiler.<anonymous> (C:\work\VSO\vso\node_modules\webpack\lib\CachePlugin.js:78:5)

That the manifest file is written in node code by fs-extra. A cursory examination leads me to believe that the code is technically correct (when running on Windows) since it is converting a file object to a string.

An alternative approach might be to get the node dependent library to support a flag to force UNIX style slashes.

The hash duplicates keys so, if running on a windows box, both look ups succeed:
'''
<%=stylesheet_pack_tag 'stylesheets/layout' %>
'''
and
'''
<%=stylesheet_pack_tag "stylesheets\layout" %>
'''
Although I doubt any windows people would take the second approach (but both were tested in a deployed war file running in production mode).

I don't have a unix box at the moment to do that level of sanity testing.

cshupp added 2 commits January 22, 2018 14:10
Adding in tzinfo so 'bundle exec rake' can run tests on Windows.
6da8f39
Fix for:
7b87c63 
rails#1174

If the build was done on windows then manifest.json will have windows style slashes in the keys.  We duplicate those enteries, with unix style slashes, to ensure lookups succeed.
gauravtiwari
gauravtiwari reviewed Jan 22, 2018
View reviewed changes
lib/webpacker/manifest.rb Outdated
def compiling?
config.compile? && !dev_server.running?
end
def compiling?
Copy link
Member

Choose a reason for hiding this comment

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

Please can you revert indentation change. All rails gems use this style:

private
  def compiling?
  end

Copy link
Member

Choose a reason for hiding this comment

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

May be just run bundle exec rubocop -a to fix

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK - will do

Copy link
Contributor Author

Choose a reason for hiding this comment

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

RubyMine get whiny about double quotes when singles will do... Interesting

gauravtiwari
gauravtiwari reviewed Jan 22, 2018
View reviewed changes
lib/webpacker/manifest.rb Outdated
end
end
end
end
Copy link
Member

Choose a reason for hiding this comment

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

A new line in the end please

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will do

gauravtiwari
gauravtiwari reviewed Jan 22, 2018
View reviewed changes
Gemfile Outdated
@@ -6,6 +6,8 @@ gem "rails"
gem "rake", ">= 11.1"
gem "rubocop", ">= 0.49", require: false
gem "rack-proxy", require: false
# Windows does not include zoneinfo files, so bundle the tzinfo-data gem
gem 'tzinfo-data', platforms: [:mingw, :mswin, :x64_mingw, :jruby]
Copy link
Member

Choose a reason for hiding this comment

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

Don't think we need this inside gem

Copy link
Contributor Author

Choose a reason for hiding this comment

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

People contributing on windows cannot run 'bundle exec rake' w/o it.

Copy link
Member

Choose a reason for hiding this comment

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

Ahh okay makes sense 👍 Double quotes please

gem "tzinfo-data", platforms: [:mingw, :mswin, :x64_mingw, :jruby]

gauravtiwari
gauravtiwari reviewed Jan 22, 2018
View reviewed changes
lib/webpacker/manifest.rb Outdated
data = JSON.parse config.public_manifest_path.read
#if the build was done on a windows box manifest.json will have backslashes as keys
#we will ensure lookups continue to succeed
data.keys.each do |key|
Copy link
Member

Choose a reason for hiding this comment

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

Shall we wrap this code inside? if Gem.win_platform?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, it has nothing to do where the code is running. Only where the build was run.

Copy link
Member

Choose a reason for hiding this comment

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

Oh yes ofcourse 👍

@guilleiguaran
Copy link
Member

I would prefer to get this fixed upgrading the upstream library causing the problem (shellscape/webpack-manifest-plugin#78) instead of fixing it on webpacker side.

@cshupp1
Copy link
Contributor Author

cshupp1 commented Jan 22, 2018

Me too, hence my comment:

An alternative approach might be to get the node dependent library to support a flag to force UNIX style slashes.

I don't suppose we can keep this bug open until the dependent library (with the fix) is mated to this gem so I know when to update?

@guilleiguaran
Copy link
Member

guilleiguaran commented Jan 22, 2018 •
edited
Loading

Closing this but #1174 will be open until we upgrade the dependency.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gauravtiwari gauravtiwari gauravtiwari left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@cshupp1 @guilleiguaran @gauravtiwari