Opened 14 months ago

Last modified 3 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, geo 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 (23)

comment:1 Changed 14 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 14 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 14 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 14 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 14 months ago by agiammarchi (previous) (diff)

comment:5 Changed 13 months ago by tlucas

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

comment:6 Changed 13 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 13 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 13 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 10 months ago by oleksandr

Is there an ETA for when this could be fixed?

comment:10 Changed 10 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 10 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 10 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 10 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 10 months ago by agiammarchi (previous) (diff)

comment:14 Changed 10 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 10 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 10 months ago by oleksandr

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

comment:17 Changed 10 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 10 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?

comment:19 Changed 3 months ago by greiner

Considering the last comment, is this ticket still relevant or can we close it? Because unless the requirement to support Windows becomes explicit there will be further deviations from compatibility with Windows which will make it harder to establish support.

comment:20 Changed 3 months ago by sebastian

  • Cc geo added

I would still love to see native Windows support. Currently, in our README we instruct developers on Windows to set up a WSL environment, install the build dependencies in there, and run build.py (which in turn calls npm install in adblockplusui) from within Bash. However, npm test (in order to automate browsers installed on Windows) still needs to be executed from the native Windows environment (i.e. cmd.exe or PowerShell), where therefore Node.js (and our npm dependencies) need to be available as well. All of this just so that you can use UNIX shell script syntax in your npm scripts.

comment:21 Changed 3 months ago by greiner

Ok, in that case it seems that we'd need to

  1. Translate commands in package.json to JavaScript
  2. Look into whether it'd be easier/worthwhile keeping "npm-dollar" around or replacing it with a Node-based build system
  3. Adapt existing Node scripts to make them OS-agnostic

For that, we could prioritize use cases that are relevant to anyone outside the UI team (i.e. postinstall).

Does that sound about right?

comment:22 Changed 3 months ago by agiammarchi

FWIW, we execute npm test in our CI and it has nothing to do with browsers so I am not sure I am understanding the issue, 'cause it looks not related to UI.

Bash in package.json is a super common language and there are tons of ways to work around this, let's please at lest be sure changes are needed on our side, since WSL is (IMHO) the way to go and every Windows dev would, at some point, be there, which is why it's awesome, not something to avoid (still IMHO).

Just to clarify, I'm not pushing back changes, I'm just saying we, UI, have used with ease bash and we are the one that work on UI the most so changing everything would not bring us much.

comment:23 Changed 3 months ago by agiammarchi

P.S. I've written how to run UI in WSL and if you setup a command in .bashrc the Worg bit can be automated too.
https://medium.com/@WebReflection/testing-safari-via-bash-for-windows-js-2b7b9098d9c0

Note: See TracTickets for help on using tickets.