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

gitian: Remove Windows 32 bit build #15939

Merged
merged 2 commits into from May 9, 2019

Conversation

Projects
None yet
10 participants
@MarcoFalke
Copy link
Member

commented May 2, 2019

The Windows 32 bit build has been removed from https://bitcoincore.org/en/download/, so unless there are complaints, we don't need to build it even

@@ -31,7 +31,7 @@ script: |
set -e -o pipefail
WRAP_DIR=$HOME/wrapped
HOSTS="i686-w64-mingw32 x86_64-w64-mingw32"
HOSTS="x86_64-w64-mingw32"

This comment has been minimized.

Copy link
@luke-jr

luke-jr May 2, 2019

Member

This looks so bare. Almost makes me want to build a PPC64 Windows binary just for the sake of it. :)

This comment has been minimized.

Copy link
@MarcoFalke

MarcoFalke May 2, 2019

Author Member

Can you install windows on ppc even?

This comment has been minimized.

Copy link
@luke-jr

luke-jr May 2, 2019

Member

NT4 :p

This comment has been minimized.

Copy link
@davehamiltone

davehamiltone May 3, 2019

o dear I have been running bitcoin core on a 32-bit windows system for a few years now. Am I the last one? I'd like to upgrade to 0.18 please. Any chance of having 32-bit windows back, or is it too much bother?

This comment has been minimized.

Copy link
@luke-jr

luke-jr May 6, 2019

Member

Why can't you use the 64-bit build?

This comment has been minimized.

Copy link
@jeffrade

jeffrade May 9, 2019

Contributor

I'll defer to someone who knows more than me, but can a 64-bit build run on a 32-bit machine?

@practicalswift

This comment has been minimized.

Copy link
Member

commented May 2, 2019

Concept ACK

@laanwj

This comment has been minimized.

Copy link
Member

commented May 2, 2019

even in the unlikely case that we decide to restore it for 0.18, I think removing it for 0.19 is non-controversial
utACK faf0443

@DrahtBot

This comment has been minimized.

Copy link
Contributor

commented May 2, 2019

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #15068 (Install icon & .desktop file to XDG data by luke-jr)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

@achow101

This comment has been minimized.

Copy link
Member

commented May 2, 2019

utACK faf0443

@MarcoFalke MarcoFalke added this to the 0.19.0 milestone May 2, 2019

@practicalswift

This comment has been minimized.

Copy link
Member

commented May 3, 2019

utACK faf0443

@MarcoFalke MarcoFalke force-pushed the MarcoFalke:1904-GitianWin branch from faf0443 to faf666f May 3, 2019

@MarcoFalke

This comment has been minimized.

Copy link
Member Author

commented May 3, 2019

Removed it from depends and travis as well

@fanquake

This comment has been minimized.

Copy link
Member

commented May 6, 2019

Concept ACK

@laanwj

This comment has been minimized.

Copy link
Member

commented May 6, 2019

This was bound to be a somewhat controversial decision: however one architecture per platform is great for maintenance-heavy outlier platforms such as Windows (and MacOSX). This allows for more focus in testing and, and hopefully a better user experience and better security, in time. Cross-compilation to windows is fraught with some risks, and they're minimized by only having one toolchain (mingw-w64) to check.

If you need a wider range of architectures it's better to stick with Linux, or one of the BSDs. Another option is to use a VM. I was about to suggest WSL, but there's no 32-bit support either!

@luke-jr

This comment has been minimized.

Copy link
Member

commented May 6, 2019

I do think we should keep the Travis job around...

@DrahtBot

This comment has been minimized.

Copy link
Contributor

commented May 6, 2019

Gitian builds for commit d7d7d31 (master):

Gitian builds for commit d102d5d (master and this pull):

@bitcoin bitcoin deleted a comment from DrahtBot May 6, 2019

@fanquake

This comment has been minimized.

Copy link
Member

commented May 8, 2019

There's a few more win32 references that can be removed:

https://github.com/bitcoin/bitcoin/blame/master/doc/release-process.md#L224

subprocess.check_call('mv build/out/bitcoin-*win32-setup.exe ../bitcoin-binaries/'+args.version, shell=True)

@MarcoFalke

This comment has been minimized.

Copy link
Member Author

commented May 8, 2019

Thanks, done

RUN_FUNCTIONAL_TESTS=false
GOAL="deploy"
BITCOIN_CONFIG="--enable-reduce-exports --disable-gui-tests"

This comment has been minimized.

Copy link
@luke-jr

luke-jr May 8, 2019

Member

Let's keep the Travis job...

This comment has been minimized.

Copy link
@MarcoFalke

MarcoFalke May 8, 2019

Author Member

Why? I am not aware that this job ever failed and the win64 one didn't. Also, why would we want to waste resources on testing a target that we never ship?
I'd rather have freebsd tests than a win32 one.

This comment has been minimized.

Copy link
@luke-jr

luke-jr May 8, 2019

Member

We ship source code.

This comment has been minimized.

Copy link
@MarcoFalke

MarcoFalke May 8, 2019

Author Member

It is the responsibility of the user to run the tests. You could argue that many users don't run the tests on their target when they download the gitian binaries, but that doesn't apply here. If some users compile on their own, they need to run the tests themselves.

This comment has been minimized.

Copy link
@luke-jr

luke-jr May 8, 2019

Member

Travis doesn't exist to run the tests. It exists to help us developers avoid doing things that will break the tests. (Running the tests is just how we accomplish that.)

This comment has been minimized.

Copy link
@MarcoFalke

MarcoFalke May 8, 2019

Author Member

Either we support win32, test and ship it, or we don't. There is no in-between.

This comment has been minimized.

Copy link
@luke-jr

luke-jr May 8, 2019

Member

I do not agree. We support plenty of things we do not recommend or ship binaries for.

Indeed, the best approach is to compile yourself, which itself falls outside the "ship" scope.

This comment has been minimized.

Copy link
@sdaftuar

sdaftuar May 8, 2019

Member

FWIW I agree with @MarcoFalke here -- seems like not a great use of travis resources to explicitly test a platform that we're no longer interested in continuing to support (who will debug problems if they are found?). On top of that, reducing the load on travis has benefits; I've noticed that the time I wait from updating a PR to having the travis jobs complete has ticked up recently. So I'd rather we not spend those travis cycles on win32 unless it was an important platform to test on for some reason, and the decision to drop it strikes me as a statement that it's not.

This comment has been minimized.

Copy link
@luke-jr

luke-jr May 9, 2019

Member

The decision not to provide binaries isn't necessarily a decision to drop support.

(That being said, I certainly am not interested in providing support, so if nobody else is either...)

@fanquake

This comment has been minimized.

Copy link
Member

commented May 9, 2019

utACK fa193dc

I agree with @MarcoFalke & @sdaftuar about Travis. I don't think we should have a job dedicated to a binary we aren't shipping.

@laanwj

This comment has been minimized.

Copy link
Member

commented May 9, 2019

utACK fa193dc

@laanwj laanwj merged commit fa193dc into bitcoin:master May 9, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

laanwj added a commit that referenced this pull request May 9, 2019

Merge #15939: gitian: Remove Windows 32 bit build
fa193dc doc: Remove win32 from the release process (MarcoFalke)
faf666f Remove Windows 32 bit build (MarcoFalke)

Pull request description:

  The Windows 32 bit build has been removed from https://bitcoincore.org/en/download/, so unless there are complaints, we don't need to build it even

ACKs for commit fa193d:
  fanquake:
    utACK fa193dc

Tree-SHA512: d6f2976a2e0c407698f720b00ac23ec4056626de4eff8621f4c5581120af0460afd1bdef72329cc0e7d92afca48d94ae5fce6777cb36bfabb60b8034ff08fd88

@MarcoFalke MarcoFalke deleted the MarcoFalke:1904-GitianWin branch May 9, 2019

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request May 10, 2019

Merge bitcoin#15939: gitian: Remove Windows 32 bit build
fa193dc doc: Remove win32 from the release process (MarcoFalke)
faf666f Remove Windows 32 bit build (MarcoFalke)

Pull request description:

  The Windows 32 bit build has been removed from https://bitcoincore.org/en/download/, so unless there are complaints, we don't need to build it even

ACKs for commit fa193d:
  fanquake:
    utACK bitcoin@fa193dc

Tree-SHA512: d6f2976a2e0c407698f720b00ac23ec4056626de4eff8621f4c5581120af0460afd1bdef72329cc0e7d92afca48d94ae5fce6777cb36bfabb60b8034ff08fd88
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.