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

Add a workflow to test building from a read-only source tree #22908

Closed
wants to merge 2 commits into from
Closed

Add a workflow to test building from a read-only source tree #22908

wants to merge 2 commits into from

Conversation

levitte
Copy link
Member

@levitte levitte commented Dec 2, 2023 •
edited

... and deal with the consequences.

Fixes #22907

@levitte
Copy link
Member Author

levitte commented Dec 2, 2023

The extended job that I just added turns out to be too tough for a read-only source tree.

However, maybe that's unfair, as our release tarballs don't contain any submodule or any fuzz test data.

@nhorman
Copy link
Contributor

nhorman commented Dec 2, 2023

FWIW, I just tried this locally, but I created the read-only source tree by building a tarball (via utils/mktar.sh), and untaring the results, and that worked for me

@t8m
Copy link
Member

t8m commented Dec 4, 2023

The external tests (or at least some of them) aren't supposed to be runnable with out-of-tree builds.

Does the test need to be in a separate workflow file? I would recommend just modifying the existing out-of-source-and-install job in the ci.yml.

@levitte
Copy link
Member Author

levitte commented Dec 4, 2023 •
edited

The external tests (or at least some of them) aren't supposed to be runnable with out-of-tree builds.

Yeah, I've figured the same thing... and they do require a git checkout, so it's a bit over the top, like I mentioned. But, it was an interesting experiment. I'll remove that commit.

Does the test need to be in a separate workflow file? I would recommend just modifying the existing out-of-source-and-install job in the ci.yml.

They don't have to. Personally, I'm in favor of smaller, "on topic" files, rather than piling a whole bunch of mixed stuff in one file. ci.yml is already pretty stuffed...

@levitte levitte force-pushed the workflow-readonly-source branch 2 times, most recently from 78e1fe8 to efa1065 Compare December 4, 2023 09:38
@t8m
Copy link
Member

t8m commented Dec 4, 2023

Does the test need to be in a separate workflow file? I would recommend just modifying the existing out-of-source-and-install job in the ci.yml.

They don't have to. Personally, I'm in favor of smaller, "on topic" files, rather than piling a whole bunch of mixed stuff in one file. ci.yml is already pretty stuffed...

But in fact this job does not add anything to the existing workflow than just making the source tree read-only. IMO it is a waste of resources to add another job just for this.

@levitte
Copy link
Member Author

levitte commented Dec 4, 2023

I'm not very attached to the separate workflow file, so I've done a fixup that works along your line of thought, @t8m

@t8m
Copy link
Member

t8m commented Dec 4, 2023

Thank you! This looks good - the CI passed. Is it ready for review now?

@t8m
Copy link
Member

t8m commented Dec 4, 2023

Also, should this go in master branch only or all active branches?

@t8m t8m added approval: review pending This pull request needs review by a committer approval: otc review pending This pull request needs review by an OTC member triaged: bug The issue/pr is/fixes a bug tests: present The PR has suitable tests present branch: master Merge to master branch labels Dec 4, 2023
@levitte
Copy link
Member Author

levitte commented Dec 4, 2023

I personally think it should go to all release branches. Hmmm, gotta check if this cherry-picks cleanly (a separate workflow file would have diminished the risk of conflicts 😉)

@levitte
Copy link
Member Author

levitte commented Dec 4, 2023

I'll have to make a backport PR for 3.1 and 3.0

@levitte levitte added the branch: 3.2 Merge to openssl-3.2 label Dec 4, 2023
@levitte levitte marked this pull request as ready for review December 4, 2023 12:21
@mspncp
Copy link
Contributor

mspncp commented Dec 4, 2023

If it's ready for review, you might want to remove the [WIP] from the title?

@levitte levitte changed the title [WIP] Add a workflow to test building from a read-only source tree Add a workflow to test building from a read-only source tree Dec 4, 2023
@levitte
Copy link
Member Author

levitte commented Dec 4, 2023

If it's ready for review, you might want to remove the [WIP] from the title?

Yes. Thank you. Done

@levitte levitte added severity: regression The issue/pr is a regression from previous released version and removed severity: regression The issue/pr is a regression from previous released version labels Dec 4, 2023
@levitte levitte mentioned this pull request Dec 4, 2023
Closed
@levitte
Copy link
Member Author

levitte commented Dec 4, 2023

I have verified that this cherry-picks cleanly to openssl-3.2

@t8m t8m removed the approval: otc review pending This pull request needs review by an OTC member label Dec 4, 2023
tmshort
tmshort approved these changes Dec 5, 2023
View reviewed changes
Copy link
Contributor

@tmshort tmshort left a comment

Choose a reason for hiding this comment

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

LGTM

@tmshort tmshort added approval: done This pull request has the required number of approvals and removed approval: review pending This pull request needs review by a committer labels Dec 5, 2023
@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 Dec 6, 2023
@openssl-machine
Copy link
Collaborator

This pull request is ready to merge

@levitte
Copy link
Member Author

levitte commented Dec 7, 2023

Merged

master:
266a355 Modify 'out-of-source-and-install' to work with a read-only source tree
504ff2a Configure: Refuse to make directories in the source tree

3.2:
95a99fd Modify 'out-of-source-and-install' to work with a read-only source tree
96b8bc7 Configure: Refuse to make directories in the source tree

@levitte levitte closed this Dec 7, 2023
openssl-machine pushed a commit that referenced this pull request Dec 7, 2023
@levitte
Modify 'out-of-source-and-install' to work with a read-only source tree
266a355 
This also adds the configuration options 'enable-quic'.

Fixes #22907

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Todd Short <todd.short@me.com>
(Merged from #22908)
openssl-machine pushed a commit that referenced this pull request Dec 7, 2023
@levitte
Configure: Refuse to make directories in the source tree
504ff2a 
Fixes #22907

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Todd Short <todd.short@me.com>
(Merged from #22908)
openssl-machine pushed a commit that referenced this pull request Dec 7, 2023
@levitte
Modify 'out-of-source-and-install' to work with a read-only source tree
95a99fd 
This also adds the configuration options 'enable-quic'.

Fixes #22907

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Todd Short <todd.short@me.com>
(Merged from #22908)

(cherry picked from commit 266a355)
openssl-machine pushed a commit that referenced this pull request Dec 7, 2023
@levitte
Configure: Refuse to make directories in the source tree
96b8bc7 
Fixes #22907

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Todd Short <todd.short@me.com>
(Merged from #22908)

(cherry picked from commit 504ff2a)
@levitte levitte deleted the workflow-readonly-source branch December 7, 2023 06:56
wanghao75 pushed a commit to openeuler-mirror/openssl that referenced this pull request Dec 15, 2023
@levitte
Modify 'out-of-source-and-install' to work with a read-only source tree
fb3825d 
This also adds the configuration options 'enable-quic'.

Fixes #22907

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Todd Short <todd.short@me.com>
(Merged from openssl/openssl#22908)

Signed-off-by: fly2x <fly2x@hitls.org>
wanghao75 pushed a commit to openeuler-mirror/openssl that referenced this pull request Dec 15, 2023
@levitte
Configure: Refuse to make directories in the source tree
ef98c24 
Fixes #22907

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Todd Short <todd.short@me.com>
(Merged from openssl/openssl#22908)

Signed-off-by: fly2x <fly2x@hitls.org>
wanghao75 pushed a commit to openeuler-mirror/openssl that referenced this pull request Dec 15, 2023
@levitte
Modify 'out-of-source-and-install' to work with a read-only source tree
b9ca625 
This also adds the configuration options 'enable-quic'.

Fixes #22907

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Todd Short <todd.short@me.com>
(Merged from openssl/openssl#22908)

(cherry picked from commit 266a3553d743f5335ccdff196a07916f03d34d0d)
Signed-off-by: fly2x <fly2x@hitls.org>
wanghao75 pushed a commit to openeuler-mirror/openssl that referenced this pull request Dec 15, 2023
@levitte
Configure: Refuse to make directories in the source tree
5ee4247 
Fixes #22907

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Todd Short <todd.short@me.com>
(Merged from openssl/openssl#22908)

(cherry picked from commit 504ff2a4ef5f26990a48ca3d664ac1e5d9cb20b9)
Signed-off-by: fly2x <fly2x@hitls.org>
wbeck10 pushed a commit to wbeck10/openssl that referenced this pull request Jan 8, 2024
@levitte @wbeck10p
Modify 'out-of-source-and-install' to work with a read-only source tree
08176ae 
This also adds the configuration options 'enable-quic'.

Fixes openssl#22907

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Todd Short <todd.short@me.com>
(Merged from openssl#22908)
wbeck10 pushed a commit to wbeck10/openssl that referenced this pull request Jan 8, 2024
@levitte @wbeck10p
Configure: Refuse to make directories in the source tree
19b69a3 
Fixes openssl#22907

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Todd Short <todd.short@me.com>
(Merged from openssl#22908)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@t8m t8m t8m approved these changes

@tmshort tmshort tmshort approved these changes

Assignees
No one assigned
Labels
approval: ready to merge The 24 hour grace period has passed, ready to merge branch: master Merge to master branch branch: 3.2 Merge to openssl-3.2 tests: present The PR has suitable tests present triaged: bug The issue/pr is/fixes a bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

We're not treating read-only source trees very well
6 participants
@levitte @nhorman @t8m @mspncp @openssl-machine @tmshort