Opened 10 months ago

Last modified 6 months ago

#6587 new defect

adblockplusui builds on Windows are broken

Reported by: oleksandr Assignee:
Priority: Unknown Milestone:
Module: User-Interface Keywords:
Cc: juliandoucette, greiner, saroyanm, sebastian, agiammarchi, BrentM Blocked By:
Blocking: Platform: Edge
Ready: no Confidential: no
Tester: Unknown Verified working: no
Review URL(s):

Description

Environment

Windows 10, Fall Creators Update

How to reproduce

  1. clone adblockplusui
  2. run npm install
  3. observe an error:

npm ERR! Failed at the adblockplusui@ bash:js script 'eslint ./js/**/*.js && bash -c 'echo "/* eslint-disable */$(browserify --node --no-bundle-external $0)">$1' "./js/desktop-options.js" "./desktop-options.js"'.

Expected behaviour

There is no bash on Windows, so we should replace that with cmd.

Change History (18)

comment:1 Changed 10 months ago by sebastian

  • Cc sebastian agiammarchi added

Alternatively, we could write a JS script which then can run in any environment.

comment:2 Changed 10 months ago by agiammarchi

This is a very valid point. I have assumed Windows builds would've run under Linux, macOS, WSL Bash, or git-shell for Windows console, which is a quite standard environment and most of the bash in there "just works".

If that is not the case, and builds for Windows always run on regular windows, I agree with @sebastian we need a JS script that would work in there too, which is preferable than cmd and more appropriate for a JSON file (no need to detect and branch inline).

comment:3 Changed 10 months ago by greiner

That sounds very much like #6303 (part of #6117) which is still open. There we mention using "gulp" for orchestration.

@Andrea Is #6303 supposed to be closed since a commit has already landed or do you prefer we keep it open for this?

comment:4 Changed 10 months ago by agiammarchi

@Thomas I forgot to close that, which I've done now. That was about basics, which are in. Now basics need to be improved. I'd like to have my current modules in review out before though, otherwise will be a mess if we change things with many reviews based on that approach already.

I also kinda think if we have to move to an orchestrator then we might be better off hooking ourselves into parent folder Webpack (or change the build tools to bundle us as well)

Having Gulp in a subfolder handled by Webpack seems a bit odd to me.

Last edited 10 months ago by agiammarchi (previous) (diff)

comment:5 Changed 9 months ago by tlucas

FYI - Brent (getadblock.com) just ran into this issue, preventing proper development on Windows.

comment:6 Changed 9 months ago by agiammarchi

@tlucas can Brent generate builds for Windows using same stack we do? (Linux / macOS) AFAIK there is not much of a priority to change this in the short term so I'm trying to understand if we should, instead, prioritize (considering the imminent code freeze too).

Thanks in advance for expanding about this.

comment:7 Changed 9 months ago by sebastian

  • Cc BrentM added

As far as I can tell, its not a super blocker. Building Adblock Plus with Bash on Ubuntu on Windows does as a workaround, and even if we get a fix in Adblock Plus 3.2, AdBlock will likely update the code base anyway before Adblock Plus 3.2 is released (using one workaround or another).

However, if this issue is all that is preventing Adblock Plus from being built successfully on native Windows, than we should just address it. Also our own developers who use Windows would much prefer to be able to build Adblock Plus in a native environment, not to mention confused contributors.

comment:8 Changed 9 months ago by agiammarchi

This is surely something that needs to be addressed at some point, but since Windows has various options for developers (WSL, Git shell, MinGW, others) and all of them would build natively for Windows, we'd like to not rush any workaround (and rather take time to properly implement a Webpack configuration in a way that is future-compatible with our parent folder).

comment:9 Changed 6 months ago by oleksandr

Is there an ETA for when this could be fixed?

comment:10 Changed 6 months ago by agiammarchi

I think the main issue here is that cmd is not a bash substitute but Windows has many ways to run bash, including WLS, Git for Windows, others such chocolatey where you can simply choco install git to have usable bash everywhere.

Since Windows developers are rare but also capable of installing bash, which is usually always available since it's the most used nodejs tool for scripts, why should we fix this or how much important this really is?

comment:11 Changed 6 months ago by oleksandr

Currently this is the only issue that prevents us to build ABP for Microsoft Edge on Windows using native Windows tools. This is rather unfortunate, and we have made great effort in past to ensure Windows developers aren't treated as second class citizens.

Right now we can get by with WSL and such, but even using ManifoldJS for producing .appx package works differently on Windows and Linux. I think it is only a matter of time when this will bite us. Frankly, I don't see enough reasons for effectively dropping support of builds on Windows.

And Sebastian's point about third-party contributors is valid too.

comment:12 Changed 6 months ago by agiammarchi

Frankly, I don't see enough reasons for effectively dropping support of builds on Windows.

I don't think it's our plan to officially drop support, and it would just take a lot of time to change current settings so it's nowhere in our schedule because none of us uses Windows, no external contributor ever complained (so far) but also everything works via WSL and/or bash for git and others solutions.

I guess my answer, AFAIK, is simply that there is no ETA for now, and we are shipping soon tons of changes with a lot of high priority tickets, so I don't know when we'll have time to change our build tools, sorry.

Maybe @greiner has more ideas when this might happen.

comment:13 Changed 6 months ago by agiammarchi

P.S. just thinking that having NodeJS is a requirement to build ABP so what if we simply make Bash a requirement too, providing links / solutions in the ABP UI readme? Would that be a better workaround for now?

Last edited 6 months ago by agiammarchi (previous) (diff)

comment:14 Changed 6 months ago by oleksandr

no external contributor ever complained

Well, from the comments above Brent apparently complained. Inside the company, I use Windows, and I am not sure what Georgiana is using, but it's not a question of what we use, but of what we support. I agree it shouldn't top your priorities, of course, just trying to put this somewhere on the roadmap.

simply make Bash a requirement too, providing links / solutions in the ABP UI readme

Yes, it's ok for now, but instructions would also need to be added in adblockpluschrome. FWIW just running npm install in Bash once after checkout within adblockplusui directory is enough to keep working with native tools.

comment:15 Changed 6 months ago by agiammarchi

by any chance you have experience with this NodeJS library ?
https://github.com/shelljs/shx

if yes, do you think that might be a solution?

comment:16 Changed 6 months ago by oleksandr

Never used it myself, but looks like exactly what we need here!

comment:17 Changed 6 months ago by BrentM

Our team definitely does development work on Windows 10 (and mac os), so a long term solution is desirable.

I glanced at the ShellJS and Shx (I haven't used them either), but it seems to be the missing library that's needed.

comment:18 Changed 6 months ago by agiammarchi

Update

I gave ShellJS Shx a chance, but it's not as powerful as I hoped. Most basic needed parts such passed arguments are ignored so that everything throws "like a charm".

However, since Git for Windows automatically install, by default, a usable bash executable, I wonder if the following could be the solution:

npm config set script-shell "C:\\Program Files (x86)\\git\\bin\\bash.exe"

Being sure that it's either C:\\Program Files (x86)\\git\\bin\\bash.exe or C:\\Program Files\\git\\bin\\bash.exe.

That would align every script based setup (tons of them in GitHub) to use Bash instead of cmd as suggested already long long time ago in SO https://stackoverflow.com/questions/23243353/how-to-set-shell-for-npm-run-scripts-in-windows

TL;DR it looks like Windows developers already have bash, they just don't know/use it yet in conjunction with npm. Could this be a solution?

Note: See TracTickets for help on using tickets.