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

Only run -dip1000 checks on CircleCi on not on the auto-tester #6443

Merged
merged 1 commit into from
May 3, 2018

Conversation

wilzbach
Copy link
Member

It seems that there are still many issues with -dip1000 and for the time being it's apparentely better to only test the separate .test targets with -dip1000.
This is not ideal as -dip1000 is now only checked on Linux 64-bit, but before it wasn't run on Windows neither and -dip1000 is not very platform dependent.

This should hopefully unblock dlang/dmd#8124

The only downside to this is that now a PR at dmd will no longer run Phobos with -dip1000 checks. We can either

  • hope that no one will regress the CircleCi dip1000-enabled *.test
  • let dmd's CircleCi run the *.test targets too
  • fix the mangling issues with -dip1000 and re-enable the checking that this PR disables

@dlang-bot
Copy link
Contributor

Thanks for your pull request, @wilzbach!

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 + phobos#6443"

posix.mak Outdated
@@ -348,7 +348,7 @@ UT_D_OBJS:=$(addprefix $(ROOT)/unittest/,$(addsuffix .o,$(D_MODULES)))
$(UT_D_OBJS): $(ALL_D_FILES)
$(UT_D_OBJS): $(ROOT)/unittest/%.o: %.d
@mkdir -p $(dir $@)
$(DMD) $(DFLAGS) $(UDFLAGS) $(aa[$(subst /,.,$(basename $<))]) -c -of$@ $<
$(DMD) $(DFLAGS) $(UDFLAGS) $(basename $<) -c -of$@ $<
Copy link
Member

Choose a reason for hiding this comment

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

That $(basename $<) seems to cause issues.

Copy link
Contributor

@marler8997 marler8997 Apr 12, 2018

Choose a reason for hiding this comment

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

Yeah, remove $(basename $<) completely

@marler8997
Copy link
Contributor

@wilzbach did you see our comments?

@schveiguy
Copy link
Member

@marler8997 Just realized I could edit it. We'll see if that helps.

@marler8997
Copy link
Contributor

How does editing other people's PRs work? Did you just push to the PR branch?

git push https://github.com/wilzbach/phobos HEAD:dip1000-unblock -f

@schveiguy
Copy link
Member

You have to be a member. I just edit it directly in github (I know there's other ways, but I'm not familiar enough with how git works).

@marler8997
Copy link
Contributor

It looks like Seb's branch has your commit on top of his. So I guess when you make a PR, you are granting push rights to the repo you are pushing to. Very interesting use case...

@schveiguy
Copy link
Member

So I guess when you make a PR, you are granting push rights to the repo you are pushing to.

Yes, it's a box that's checked by default. See here: https://help.github.com/articles/allowing-changes-to-a-pull-request-branch-created-from-a-fork/

Seb knows how to do it without going "on top" of the original, but I have no clue how to do that :)

@marler8997
Copy link
Contributor

Seb knows how to do it without going "on top" of the original, but I have no clue how to do that :)

The command I sent you should do the trick. Just amend his commit and do a force push to his branch.

@schveiguy
Copy link
Member

@wilzbach same error in the doc build that I'm seeing on #6453

@wilzbach
Copy link
Member Author

FWIW I have a simple script that automates checking out and pushing to any PR:
https://github.com/wilzbach/git-tools

So it looks like the error is unrelated... great :/
I guess we can/have to remove the example from the spec tester until we have figured out what's going on there. I will get to this later tonight. Sorry.

@marler8997
Copy link
Contributor

Your tools are in bash :( If I decide to use them maybe I'll convert them to D :)

@marler8997
Copy link
Contributor

P.S. I have one custom git tool that I use for pull requests

https://github.com/marler8997/utils

git fetchout <repo> <branch>

It fetches the branch from <repo> to a local branch of the same name. If it already exists it will prompt to see if you want to override the local branch, then it checks out that branch.

git fetchout marler8997 myCoolFeature

@wilzbach
Copy link
Member Author

Regarding the DAutoTest failure -> dlang/dlang.org#2345

(I also squashed the commits as discussed here and to restart DAutoTest)

@wilzbach did you see our comments?

Yes, but the new uni semester started this week and I was rather busy. Sorry.
Glad you almost managed to get it to work without me :)

@marler8997
Copy link
Contributor

School...bleh.

Majoring in compter science I assume? How's that going?

@wilzbach
Copy link
Member Author

wilzbach commented May 2, 2018

Majoring in compter science I assume? How's that going?

My major actually is computational biology, so well not entirely on track ;-)

Anyhow this is passing now, so I assume you still want this for your dmd PR.
Anyone around for a quick pull?

@schveiguy
Copy link
Member

There you go. Should merge soon.

@wilzbach
Copy link
Member Author

wilzbach commented May 2, 2018

@schveiguy auto-merge isn't working because this PR doesn't have an approval yet.

@wilzbach
Copy link
Member Author

wilzbach commented May 3, 2018

Merging can be performed automatically with 1 approving review.

@dlang-bot dlang-bot merged commit 6c11b77 into dlang:master May 3, 2018
@schveiguy
Copy link
Member

Oops!

@schveiguy
Copy link
Member

schveiguy commented May 3, 2018

@marler8997 what @wilzbach meant is that someone with commit rights must approve.

@WalterBright
Copy link
Member

mangling issues with -dip1000

What's the bugzilla issue for that? It seems to be mis-categorized, as I can't find it.

@schveiguy
Copy link
Member

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.

5 participants