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

[build] Remove unemployed 'skip-{build,test}-*' flags from build-script-impl #33154

Merged
merged 1 commit into from Aug 11, 2020

Conversation

finagolfin
Copy link
Contributor

These flags have done nothing for awhile, as 26 of 31 have already been moved to build-script. Only the flags related to haiku, maccatalyst, and external-benchmarks are unrecognized by either build script after this removal.

I just checked to see if --skip-build-linux would disable building all products for that platform, as I've been doing manually when cross-compiling the toolchain for Android, and was surprised to find that the flag did nothing in build-script-impl. I investigated further and found that these 30 flags do nothing too, as most of them have been moved to build-script and their functionality removed from the bash script, eg #26495.

The haiku-related functionality has been swept aside over time, while maccatalyst never did anything, maybe because the actual implementation hasn't been open-sourced? I noticed that another flag from that Catalyst pull does nothing, but didn't remove it here.

@gottesmm, all the implementation of --build-external-benchmarks has been moved to build-script and the Python scripts. You could add back a --skip-build-external benchmarks there that actually works, but since the default is to skip it, probably not needed.

@Rostepher, review requested.

…pt-impl

These flags have done nothing for awhile, as 26 of 31 have already been moved to
build-script. Only the flags related to haiku, maccatalyst, and external-benchmarks
are unrecognized by either build script after this removal.
@Rostepher
Copy link
Contributor

@edymtt can probably review this change. I'm no longer working on the Swift project.

@finagolfin
Copy link
Contributor Author

@bitjammer, looks like you added a lot of these flags, review requested.

@bitjammer
Copy link
Member

I haven't edited this file since it was reorganized some years ago. I recommend asking @shahmishal if he or someone else can review on this one. Thanks!

@shahmishal
Copy link
Member

@swift-ci smoke test

@shahmishal
Copy link
Member

@swift-ci build toolchain

@Rostepher Rostepher requested a review from edymtt July 28, 2020 16:57
@swift-ci
Copy link
Collaborator

Linux Toolchain (Ubuntu 16.04)
Download Toolchain
Git Sha - 587973f

Install command
tar zxf swift-PR-33154-409-ubuntu16.04.tar.gz
More info

@swift-ci
Copy link
Collaborator

macOS Toolchain
Download Toolchain
Git Sha - 587973f

Install command
tar -zxf swift-PR-33154-585-osx.tar.gz --directory ~/

@compnerd
Copy link
Collaborator

@swift-ci please smoke test Linux platform

@gottesmm
Copy link
Member

@buttaface You can remove all of the cmake build benchmark external.

Copy link
Member

@gottesmm gottesmm left a comment

Choose a reason for hiding this comment

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

A few questions.

@@ -560,9 +560,6 @@ class BuildScriptInvocation(object):
impl_args += ["--skip-build"]
if not args.build_benchmarks:
impl_args += ["--skip-build-benchmarks"]
# Currently we do not build external benchmarks by default.
if args.build_external_benchmarks:
Copy link
Member

Choose a reason for hiding this comment

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

This can go.

impl_args += ["--skip-build-freebsd"]
if not args.build_cygwin:
impl_args += ["--skip-build-cygwin"]
if not args.build_osx:
Copy link
Member

Choose a reason for hiding this comment

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

Does this really not do anything? I am pretty sure I have used these in the past bit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If invoked originally from the Python build-script, it hasn't done anything in this bash script in many years, as it's handled in Python, #2880. As a result, the actual implementation of this flag in this bash script was removed last year in the aforementioned pull #26495.

@finagolfin
Copy link
Contributor Author

You can remove all of the cmake build benchmark external.

All bash flags in this pull were removed because I could find no trace of them being used in the bash script build-script-impl, ie none are used anywhere or passed to any of the CMake invocations in the script. All usage of these flags has long since been removed, I'm just taking the last step of removing the unused flags.

@finagolfin
Copy link
Contributor Author

Ping, ready to go?

@finagolfin
Copy link
Contributor Author

Ping @shahmishal, ready to merge, though you may need to rerun the CI with the latest commit from the main branch first.

@shahmishal
Copy link
Member

@swift-ci test and merge

@swift-ci swift-ci merged commit 2a5af15 into apple:master Aug 11, 2020
@finagolfin finagolfin deleted the flags branch August 12, 2020 02:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants