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

Remove configure script #1657

Merged
merged 4 commits into from
May 17, 2021
Merged

Remove configure script #1657

merged 4 commits into from
May 17, 2021

Conversation

lava
Copy link
Member

@lava lava commented May 12, 2021

📔 Description

To quote the corresponding story, the main reasons for removing:

  • The script is called configure but does not act like a standard configure script, leading to confusion for people who just want to plug the build into some automated system with hooks. We usually have to tell them "please just ignore the configure script, we are actually using cmake as build system, yes we know its confusing".
    • It does not operate in the current directory by default
    • It overwrites the contents of any existing directory named build/ by default
    • It does not understand all the same default options that autotools-generated configure scripts have
  • Its easy to forget to add new features to the configure script, leading to a divergence over time
  • It slows down development since we have to invest time in updating it every time we touch the build system.

📝 Checklist

  • All user-facing changes have changelog entries.
  • The changes are reflected on docs.tenzir.com/vast, if necessary.
  • The PR description contains instructions for the reviewer, if necessary.

🎯 Review Instructions

@lava lava force-pushed the story/ch25485/remove-configure branch from 803da6c to c73fd69 Compare May 12, 2021 08:38
Copy link
Member

@dominiklohmann dominiklohmann left a comment

Choose a reason for hiding this comment

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

Love to see this done so much that I am commenting this on my day off. 🎉

.github/workflows/vast.yaml Outdated Show resolved Hide resolved
.github/workflows/vast.yaml Outdated Show resolved Hide resolved
.github/workflows/vast.yaml Show resolved Hide resolved
changelog/unreleased/breaking-changes/1657.md Show resolved Hide resolved
scripts/create-release Outdated Show resolved Hide resolved
@lava lava force-pushed the story/ch25485/remove-configure branch 3 times, most recently from ccc0e70 to 17cbd5c Compare May 12, 2021 09:21
@JoeLoser
Copy link
Contributor

I like this. Is there any plans to remove scripts use of configure? E.g. https://github.com/tenzir/vast/blob/a2cb4be879a13cef855da2c1d73083204aed4dff/scripts/clang-build-analyzer#L37 and perhaps others.

@lava
Copy link
Member Author

lava commented May 14, 2021

@JoeLoser Absolutely, I used git grep to search for any occurences in the scripts but I must have missed this one.

@mavam mavam added the maintenance Tasks for keeping up the infrastructure label May 15, 2021
.github/workflows/vast.yaml Outdated Show resolved Hide resolved
.github/workflows/vast.yaml Outdated Show resolved Hide resolved
.github/workflows/vast.yaml Show resolved Hide resolved
.github/workflows/vast.yaml Outdated Show resolved Hide resolved
.github/workflows/vast.yaml Show resolved Hide resolved
scripts/clang-build-analyzer Outdated Show resolved Hide resolved
Dockerfile Show resolved Hide resolved
Dockerfile Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@lava lava requested a review from dominiklohmann May 17, 2021 11:24
.github/workflows/vast.yaml Outdated Show resolved Hide resolved
INSTALLATION.md Show resolved Hide resolved
.github/workflows/vast.yaml Show resolved Hide resolved
Dockerfile Outdated Show resolved Hide resolved
@lava lava force-pushed the story/ch25485/remove-configure branch from dd5b2b9 to 042e2aa Compare May 17, 2021 13:01
@lava lava enabled auto-merge May 17, 2021 13:02
@lava lava merged commit b9962cd into master May 17, 2021
@lava lava deleted the story/ch25485/remove-configure branch May 17, 2021 13:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintenance Tasks for keeping up the infrastructure
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants