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

Configure: don't try to be clever when configuring afalgeng #7688

Closed
wants to merge 1 commit into from

Conversation

rossburton
Copy link
Contributor

If the afalgeng is enabled then Configure tries to be clever but fails, by only
actually building afalgeng if it isn't being cross-compiled and if the current
kernel is 4.1+.

This means that everyone cross compiling, or whose builder machines are running
older kernels (not uncommon for build farms), can't enable afalgeng.

Instead remove the cleverness and simply enable/disable as requested.

Fixes #7687

@openssl-machine openssl-machine added the hold: cla required The contributor needs to submit a license agreement label Nov 22, 2018
@richsalz
Copy link
Contributor

+1 to this!

@levitte
Copy link
Member

levitte commented Nov 23, 2018

Do we want to consider the possibility to enable it automatically if neither disable-afalgeng nor enable-afalgeng have been given?

The procedure to do so is quite simple, have it disabled by default ($disabled{afalgeng} == 'default'), and if it hasn't been enabled or disabled explicitly (for the latter, $disabled{afalgeng} == 'option'), run the check and modify $disabled{afalgeng} accordingly.

@mattcaswell
Copy link
Member

Close/reopen to kick CLA bot.

@mattcaswell mattcaswell reopened this Nov 27, 2018
@openssl-machine openssl-machine removed the hold: cla required The contributor needs to submit a license agreement label Nov 27, 2018
@kaduk
Copy link
Contributor

kaduk commented Dec 1, 2018

Do we want to consider the possibility to enable it automatically if neither disable-afalgeng nor enable-afalgeng have been given?

Is that the sound of a yearning for autoconf that I hear?

@levitte
Copy link
Member

levitte commented Dec 1, 2018

Is that the sound of a yearning for autoconf that I hear?

No. The needed config code is already in place, all that's needed is to only run it when no afalgeng option has been given to run it.

@t8m t8m added branch: 1.1.1 Merge to OpenSSL_1_1_1-stable branch branch: master Merge to master branch triaged: bug The issue/pr is/fixes a bug labels Jul 21, 2021
@openssl-machine openssl-machine added the hold: cla required The contributor needs to submit a license agreement label Jan 27, 2022
@t8m t8m added the branch: 3.0 Merge to openssl-3.0 branch label Jan 28, 2022
@t8m
Copy link
Member

t8m commented Jan 28, 2022

As this is mostly removing useless code, we could accept this with CLA: trivial if you put the CLA: trivial on a separate line in the commit message.

@t8m t8m added the approval: review pending This pull request needs review by a committer label Jan 28, 2022
@github-actions github-actions bot added the severity: fips change The pull request changes FIPS provider sources label Jan 28, 2022
@openssl-machine openssl-machine removed the hold: cla required The contributor needs to submit a license agreement label Jan 28, 2022
If the afalgeng is enabled then Configure tries to be clever but fails,
by only actually building afalgeng if it isn't being cross-compiled and
if the current kernel is 4.1+.

This means that everyone cross compiling, or whose builder machines are
running older kernels (not uncommon for build farms), can't enable
afalgeng.

Instead remove the cleverness and simply enable/disable as requested.

Fixes openssl#7687

CLA: trivial

Signed-off-by: Ross Burton <ross.burton@arm.com>
Change-Id: I023b6cb535d5b5811823d4814fa939de3f304886
@github-actions github-actions bot removed the severity: fips change The pull request changes FIPS provider sources label Feb 2, 2022
@tom-cosgrove-arm
Copy link
Contributor

@t8m ping - Ross has added the "CLA: trivial" line

@t8m t8m added the cla: trivial One of the commits is marked as 'CLA: trivial' label Feb 16, 2022
@openssl-machine
Copy link
Collaborator

This PR is in a state where it requires action by @openssl/committers but the last update was 30 days ago

@paulidale paulidale removed the approval: review pending This pull request needs review by a committer label Mar 21, 2022
@paulidale
Copy link
Contributor

Would this benefit from a CHANGES.md entry?

@t8m
Copy link
Member

t8m commented Mar 21, 2022

Would this benefit from a CHANGES.md entry?

IMO this is fairly marginal thing, so not worth it.

@t8m t8m added the approval: done This pull request has the required number of approvals label Mar 21, 2022
@openssl-machine openssl-machine added approval: ready to merge The 24 hour grace period has passed, ready to merge and removed approval: done This pull request has the required number of approvals labels Mar 22, 2022
@openssl-machine
Copy link
Collaborator

This pull request is ready to merge

@t8m t8m added triaged: feature The issue/pr requests/adds a feature and removed branch: 1.1.1 Merge to OpenSSL_1_1_1-stable branch triaged: bug The issue/pr is/fixes a bug branch: 3.0 Merge to openssl-3.0 branch labels Mar 22, 2022
@t8m
Copy link
Member

t8m commented Mar 22, 2022

I was rethinking this and I've reconsidered my approval. I would be fine with this going into master however in the current form it should not go into stable releases.

@rossburton @tom-cosgrove-arm Are you OK with this just going into the 3.1 release?

@t8m
Copy link
Member

t8m commented Mar 22, 2022

If we wanted to put this into released branches we would have to do it the way @levitte proposed above #7688 (comment)

@rossburton
Copy link
Contributor Author

I'm fine with master only.

openssl-machine pushed a commit that referenced this pull request Mar 22, 2022
If the afalgeng is enabled then Configure tries to be clever but fails,
by only actually building afalgeng if it isn't being cross-compiled and
if the current kernel is 4.1+.

This means that everyone cross compiling, or whose builder machines are
running older kernels (not uncommon for build farms), can't enable
afalgeng.

Instead remove the cleverness and simply enable/disable as requested.

Fixes #7687

CLA: trivial

Signed-off-by: Ross Burton <ross.burton@arm.com>
Change-Id: I023b6cb535d5b5811823d4814fa939de3f304886

Reviewed-by: Paul Dale <pauli@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from #7688)
@t8m
Copy link
Member

t8m commented Mar 22, 2022

Merged to master branch. Thank you for your contribution and patience given this had to wait for so long time.

@t8m t8m closed this Mar 22, 2022
@rossburton
Copy link
Contributor Author

No problem, thanks!

@mattcaswell
Copy link
Member

This PR seems to have broken something:

https://github.com/openssl/openssl/actions/runs/2022523976

@rossburton
Copy link
Contributor Author

How did the CI for the PR work but then explode the moment it was merged! 😭

@rossburton
Copy link
Contributor Author

As that is quite a dramatic explosion, can it be reverted whilst I debug?

@t8m
Copy link
Member

t8m commented Mar 22, 2022

How did the CI for the PR work but then explode the moment it was merged!

Because we do not actually try to run the tests on cross compiled targets in the pull request CI. Running them via qemu is too slow. We run them only once the patches are merged.

IMO we just need to disable the afalgeng in our CI for the cross compile targets.

@t8m
Copy link
Member

t8m commented Mar 23, 2022

As that is quite a dramatic explosion, can it be reverted whilst I debug?

I've merged disablement of the failing test in CI. If you can find out why the test fails under qemu, it would be nice. I suppose the qemu does not translate the syscall number properly or something.

t8m pushed a commit to t8m/openssl that referenced this pull request Nov 4, 2022
If the afalgeng is enabled then Configure tries to be clever but fails,
by only actually building afalgeng if it isn't being cross-compiled and
if the current kernel is 4.1+.

This means that everyone cross compiling, or whose builder machines are
running older kernels (not uncommon for build farms), can't enable
afalgeng.

Instead remove the cleverness and simply enable/disable as requested.

Fixes openssl#7687

CLA: trivial

Signed-off-by: Ross Burton <ross.burton@arm.com>
Change-Id: I023b6cb535d5b5811823d4814fa939de3f304886

Reviewed-by: Paul Dale <pauli@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from openssl#7688)

(cherry picked from commit 9e1a54f)
t8m pushed a commit to t8m/openssl that referenced this pull request Nov 4, 2022
If the afalgeng is enabled then Configure tries to be clever but fails,
by only actually building afalgeng if it isn't being cross-compiled and
if the current kernel is 4.1+.

This means that everyone cross compiling, or whose builder machines are
running older kernels (not uncommon for build farms), can't enable
afalgeng.

Instead remove the cleverness and simply enable/disable as requested.

Fixes openssl#7687

CLA: trivial

Signed-off-by: Ross Burton <ross.burton@arm.com>
Change-Id: I023b6cb535d5b5811823d4814fa939de3f304886

Reviewed-by: Paul Dale <pauli@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from openssl#7688)

(cherry picked from commit 9e1a54f)
t8m pushed a commit to t8m/openssl that referenced this pull request Nov 9, 2022
If the afalgeng is enabled then Configure tries to be clever but fails,
by only actually building afalgeng if it isn't being cross-compiled and
if the current kernel is 4.1+.

This means that everyone cross compiling, or whose builder machines are
running older kernels (not uncommon for build farms), can't enable
afalgeng.

Instead remove the cleverness and simply enable/disable as requested.

Fixes openssl#7687

CLA: trivial

Signed-off-by: Ross Burton <ross.burton@arm.com>
Change-Id: I023b6cb535d5b5811823d4814fa939de3f304886

Reviewed-by: Paul Dale <pauli@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from openssl#7688)

(cherry picked from commit 9e1a54f)
openssl-machine pushed a commit that referenced this pull request Nov 11, 2022
If the afalgeng is enabled then Configure tries to be clever but fails,
by only actually building afalgeng if it isn't being cross-compiled and
if the current kernel is 4.1+.

This means that everyone cross compiling, or whose builder machines are
running older kernels (not uncommon for build farms), can't enable
afalgeng.

Instead remove the cleverness and simply enable/disable as requested.

Fixes #7687

CLA: trivial

Signed-off-by: Ross Burton <ross.burton@arm.com>
Change-Id: I023b6cb535d5b5811823d4814fa939de3f304886

Reviewed-by: Paul Dale <pauli@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from #7688)

(cherry picked from commit 9e1a54f)
aspeedkevin pushed a commit to AspeedTech-BMC/openssl that referenced this pull request Dec 21, 2023
If the afalgeng is enabled then Configure tries to be clever but fails,
by only actually building afalgeng if it isn't being cross-compiled and
if the current kernel is 4.1+.

This means that everyone cross compiling, or whose builder machines are
running older kernels (not uncommon for build farms), can't enable
afalgeng.

Instead remove the cleverness and simply enable/disable as requested.

Fixes #7687

CLA: trivial

Signed-off-by: Ross Burton <ross.burton@arm.com>
Reviewed-by: Paul Dale <pauli@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from openssl/openssl#7688)
Change-Id: Ibbc949222f8e9cfcb647555e5d655f90673a3491
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approval: ready to merge The 24 hour grace period has passed, ready to merge branch: master Merge to master branch cla: trivial One of the commits is marked as 'CLA: trivial' triaged: feature The issue/pr requests/adds a feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Can't enable AFALG if cross-compiling
9 participants