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

re2c: 1.0.3 -> 1.2.1 #70946

Merged
merged 2 commits into from
Oct 12, 2019
Merged

re2c: 1.0.3 -> 1.2.1 #70946

merged 2 commits into from
Oct 12, 2019

Conversation

trofi
Copy link
Contributor

@trofi trofi commented Oct 10, 2019

No description provided.

Copy link
Member

@thoughtpolice thoughtpolice left a comment

Choose a reason for hiding this comment

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

Thank you! Just one minor nit: please rebase this on top of the staging branch instead of master. This will introduce a delay (few days to ~2 weeks, IME) before you'll see 1.2.1 in master, but will save people a lot of grief by avoiding a lot of rebuilds. That's because re2c has some pretty deep transitive dependencies across the source tree, including stdenv.

A good rule of thumb (for me) is that if you're looking at ~1000 rebuilds, you might want to ask someone -- a lot of the decision can depend on the changes in question. But at ~2000 rebuilds or more in total, it should definitely go to staging. Our bot @GrahamcOfBorg will add some tags indicating how many rebuilds it estimates will happen, so you can get an idea of where it should go.

This change otherwise looks just fine -- thanks! (I'm only asking for 'request changes' to ensure someone doesn't accidentally hit the merge button before this is rebased.)

@thoughtpolice thoughtpolice self-assigned this Oct 11, 2019
@trofi trofi changed the base branch from master to staging October 11, 2019 19:10
@trofi
Copy link
Contributor Author

trofi commented Oct 11, 2019

The large rebuild impact makes sense. Rebased on to of staging.

@trofi trofi requested a review from thoughtpolice October 11, 2019 19:13
@ofborg ofborg bot added 6.topic: GNOME GNOME desktop environment and its underlying platform 6.topic: golang 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 6.topic: pantheon The Pantheon desktop environment 6.topic: printing 6.topic: python 6.topic: qt/kde 6.topic: stdenv Standard environment 8.has: documentation This PR adds or changes documentation 8.has: module (update) This PR changes an existing module in `nixos/` 2.status: merge conflict This PR has merge conflicts with the target branch and removed 6.topic: GNOME GNOME desktop environment and its underlying platform 6.topic: golang 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS labels Oct 11, 2019
@ofborg ofborg bot removed 6.topic: pantheon The Pantheon desktop environment 6.topic: printing 6.topic: python 6.topic: qt/kde 6.topic: stdenv Standard environment 8.has: documentation This PR adds or changes documentation 8.has: module (update) This PR changes an existing module in `nixos/` 2.status: merge conflict This PR has merge conflicts with the target branch labels Oct 11, 2019
Parallel builds speed up building considerably.
Tests don't add much of an overhead.

Signed-off-by: Sergei Trofimovich <slyfox@gentoo.org>
@trofi
Copy link
Contributor Author

trofi commented Oct 11, 2019

While at it enabled parallel builds and tests.

@thoughtpolice thoughtpolice merged commit 697429c into NixOS:staging Oct 12, 2019
@c0bw3b c0bw3b mentioned this pull request Oct 22, 2019
@Janik-Haag Janik-Haag added the 12. first-time contribution This PR is the author's first one; please be gentle! label Jun 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants