Skip to content
This repository was archived by the owner on Oct 12, 2022. It is now read-only.
/ druntime Public archive

Revert "Extract atomic platform specific implementation into an implementation file"#2752

Closed
CyberShadow wants to merge 1 commit intomasterfrom
revert-2739-atomic_impl
Closed

Revert "Extract atomic platform specific implementation into an implementation file"#2752
CyberShadow wants to merge 1 commit intomasterfrom
revert-2739-atomic_impl

Conversation

@CyberShadow
Copy link
Member

Reverts #2739

#2739 contains a bugfix that happened to be a breaking change. The breaking change affects vibe.d and many projects using vibe.d, thus breaking the Buildkite autotester.

A follow-up PR, #2745, contains a change which reverts the bugfix, thus unbreaking VIbe.d. However, it looks like that will take a while to fully review.

For now, we can revert #2739 to unbreak master. The submitter seems to agree with this approach.

CC @TurkeyMan

@dlang-bot
Copy link
Contributor

Thanks for your pull request, @CyberShadow!

Bugzilla references

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub fetch digger
dub run digger -- build "master + druntime#2752"

@dnadlinger
Copy link
Contributor

dnadlinger commented Aug 21, 2019

Yep; first order of business should always be to unbreak the CI, since that brings down all progress globally. This doesn't detract from the original PR, but just addresses a technical/procedural mistake (merging with CI status red).

@TurkeyMan
Copy link
Contributor

Why is buildkite still red?

Alternatively, I could just reintroduce the bug in isolation... that's a 3-line patch.

@TurkeyMan
Copy link
Contributor

Alternative approach: #2753

I don't care which one is chosen.

@CyberShadow
Copy link
Member Author

Why is buildkite still red?

Uh-oh.

It looks like a different error.

master fails with:

../../crypto/vibe/crypto/cryptorand.d(173,6): Error: cannot modify `const` expression `p`

master + this PR fails with:

../../../.dub/packages/eventcore-0.8.44/eventcore/source/eventcore/drivers/posix/processes.d(305,25): Error: undefined identifier `intptr_t`

Alternatively, I could just reintroduce the bug in isolation... that's a 3-line patch.

If it's not too much trouble.

@TurkeyMan
Copy link
Contributor

Link in comment above.

@Geod24
Copy link
Member

Geod24 commented Aug 21, 2019

Why is buildkite still red?

A new release of eventcore was tagged yesterday and that release is broken. It's already fixed: vibe-d/eventcore#120
Now @s-ludwig just needs to tag a new release of eventcore.

@CyberShadow
Copy link
Member Author

CyberShadow commented Aug 21, 2019

A new release of eventcore was tagged yesterday and that release is broken.

How did that happen? Is it broken in all D versions? Or is it broken only with D master (meaning, the new release exposed a regression in our master)?

@TurkeyMan
Copy link
Contributor

TurkeyMan commented Aug 21, 2019

I also pushed a fix to vibe.d which corrects against the bug in DMD which I fixed (vibe-d/vibe.d#2349), and that's just waiting for their CI to flush... so we could not do anything, and it will resolve when vibe.d merges the fix... too many choices.

@Geod24
Copy link
Member

Geod24 commented Aug 21, 2019

it will resolve when vibe.d merges the fix

No, it also needs a new release for Buildkite to pick it up.
And as mentioned in my other comment, it transitively breaks a lot of projects. Libraries could get the update for free, but apps that commit dub.selections.json for example will need a manual update.

How did that happen? Is it broken in all D versions?

I don't know, and asked the same question. All D versions AFAICS. Only 2.087.x.

@wilzbach (and @others): Would now be the time to make buildkite required ?
Since it's not the first time it is flat out ignored when relevant.

@wilzbach
Copy link
Contributor

Yeah I will set it as required once it's green again.

@CyberShadow
Copy link
Member Author

Merged #2753.

I understand that the remaining failures should be cleared up once the eventcore bump gets propagated to Vibe.d.

@Geod24 Geod24 deleted the revert-2739-atomic_impl branch June 2, 2021 03:41
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants