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

fix: immediately fail to compile on 32-bit systems #5237

Merged
merged 2 commits into from
Mar 15, 2023
Merged

fix: immediately fail to compile on 32-bit systems #5237

merged 2 commits into from
Mar 15, 2023

Conversation

sdbondi
Copy link
Member

@sdbondi sdbondi commented Mar 9, 2023 •
edited
Loading

Description

Fail to compile on 32-bit architectures

Motivation and Context

32-bit architectures are untested. Depending on the application, it may not compile already or there could be various classes of bugs (overflows, crashes, etc). This PR explicitly fails compilation to 32-bit targets for all applications.

How Has This Been Tested?

CI

@sdbondi sdbondi changed the title fix: fail to compile on 32-bit systems fix: immediately fail to compile on 32-bit systems Mar 9, 2023
AaronFeickert
AaronFeickert previously approved these changes Mar 9, 2023
View reviewed changes
Copy link
Collaborator

@AaronFeickert AaronFeickert left a comment

Choose a reason for hiding this comment

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

While testing a full build on a 32-bit Xubuntu 18.04 VM failed for different reasons, attempting to compile the individual libraries from this PR fails on each with the expected message.

@AaronFeickert
Copy link
Collaborator

Would it be more prudent to fail builds on anything not 64 bit, rather than only on 32 bit?

@AaronFeickert AaronFeickert mentioned this pull request Mar 9, 2023
Closed
@CjS77
Copy link
Collaborator

CjS77 commented Mar 9, 2023

If you get rustc running on an Apple ][ You deserve a fricken medal, not an error message!

@sdbondi sdbondi force-pushed the deny-32-bit-builds-for-tari-apps branch from 9d2394e to 6c645f2 Compare March 10, 2023 05:22
@sdbondi sdbondi force-pushed the deny-32-bit-builds-for-tari-apps branch from 6c645f2 to ed4cda0 Compare March 10, 2023 05:32
@sdbondi
Copy link
Member Author

sdbondi commented Mar 10, 2023

Would it be more prudent to fail builds on anything not 64 bit, rather than only on 32 bit?

Makes sense, I created a macro so that if we want to change the message/conditional we can do that in one place.
I think explicitly calling the macro is preferable to failing compilation in tari_app_utilities directly (can enable selectively, might be unexpected, explicitness).

Also added a medal 🤣

SWvheerden
SWvheerden approved these changes Mar 10, 2023
View reviewed changes
Copy link
Collaborator

@SWvheerden SWvheerden left a comment

Choose a reason for hiding this comment

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

utACK

@SWvheerden
Copy link
Collaborator

I think for now this is the correct approach, limit the end-user applications.
We have to be careful about just blocking 32-bit on all core architecture as for example the hardware wallets may need to access those and they are 32-bit only.

@stringhandler stringhandler merged commit 76aeed7 into tari-project:development Mar 15, 2023
@SWvheerden SWvheerden mentioned this pull request Mar 15, 2023
Merged
stringhandler pushed a commit that referenced this pull request Mar 15, 2023
@SWvheerden
chore(release): v0.49.0-pre.1 (#5248)
57781b6 
### ⚠ BREAKING CHANGES

* **wallet:** ensure burn shared keys and hashes match dan layer (5245)
* add claim public key to OutputFeatures (5239)
* reset esmeralda (5247)

### Features

* add claim public key to OutputFeatures
([5239](#5239))
([3e7d82c](3e7d82c))
* reset esmeralda
([5247](#5247))
([aa2a3ad](aa2a3ad))


### Bug Fixes

* added transaction revalidation to the wallet startup sequence
[5227](#5227)
([5246](#5246))
([7b4e2d2](7b4e2d2))
* immediately fail to compile on 32-bit systems
([5237](#5237))
([76aeed7](76aeed7))
* **wallet:** correct change checks in transaction builder
([5235](#5235))
([768a0cf](768a0cf))
* **wallet:** ensure burn shared keys and hashes match dan layer
([5245](#5245))
([024ce64](024ce64))
* windows path format in log4rs files
([5234](#5234))
([acfecfb](acfecfb))
@AaronFeickert AaronFeickert mentioned this pull request Jun 19, 2023
Closed
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AaronFeickert AaronFeickert AaronFeickert approved these changes

@SWvheerden SWvheerden SWvheerden approved these changes

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

Successfully merging this pull request may close these issues.
5 participants
@sdbondi @AaronFeickert @CjS77 @SWvheerden @stringhandler