Skip to content

Fix Issue 17649 - CONTRIBUTING.md instructions failed (no ../druntime… #1827

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

Merged
merged 3 commits into from
Sep 14, 2017

Conversation

wilzbach
Copy link
Member

No description provided.

@dlang-bot
Copy link
Contributor

Thanks for your pull request, @wilzbach!

Bugzilla references

Auto-close Bugzilla Description
17649 CONTRIBUTING.md instructions failed (no ../druntime dir)

@CyberShadow
Copy link
Member

This CI failure is baffling.

@wilzbach
Copy link
Member Author

Root package assert_writeln_magic references unknown package libdparse

This just means that the DUB registry is down. Sadly this happens way too often :/
Will force-push to restart.

Copy link
Member Author

@wilzbach wilzbach left a comment

Choose a reason for hiding this comment

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

CI is passing now. Anything blocking?

posix.mak Outdated
@@ -46,6 +46,10 @@ DMD_STABLE=$(DMD_STABLE_DIR)/generated/$(OS)/release/$(MODEL)/dmd
DRUNTIME_STABLE_DIR=${DRUNTIME_DIR}-${LATEST}
PHOBOS_STABLE_DIR=${PHOBOS_DIR}-${LATEST}

# Auto-closing missing directories
Copy link
Member

Choose a reason for hiding this comment

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

cloning*

@wilzbach
Copy link
Member Author

@CyberShadow I am not sure whether you noticed that I changed quite a bit of the last commit in the final rebase. I forgot to mention, but aa we auto-clone all required repos, the contributing guide can be simplified even further.

@CyberShadow
Copy link
Member

I don't see anything objectionable either way. Haven't really been paying much attention to developer documentation other than that it doesn't get too long (because then that means we have problems elsewhere) ;)

@wilzbach wilzbach merged commit 6e5fdcd into dlang:master Sep 14, 2017
of this document calls that directory henceforth `$R` from "Root". To get the
code:
If you already have a [working directory for the D language](https://wiki.dlang.org/Building_under_Posix#Fetch_repositories_from_GitHub),
e.g. `~/dlang`, change to it.
Copy link
Member

Choose a reason for hiding this comment

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

Not sure that it is a good idea to suggest the same working directory as the one install.sh manages.

Copy link
Member Author

Choose a reason for hiding this comment

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

It use ~/dlang for a long time now and since we moved the checkouts into .generated, I haven't had any problems. Though it would have been nice for the install.sh script to support an alternate directory - see dlang/installer#274

Copy link
Member

Choose a reason for hiding this comment

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

It use ~/dlang for a long time now

and I recall you said it caused problems due to conflicts between what the installer tool does and what the makefiles did.

I agree, we should change this, probably in the documentation.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry for the typo: s/It/I/

and I recall you said it caused problems due to conflicts between what the installer tool does and what the makefiles did.

Yep, it used to. However, we fixed that with #1889

I agree, we should change this, probably in the documentation.

This was an outdated comment I saw by chance and accidentally never replied, so I thought it's a good idea to leave an answer here, but I don't see any need for action anymore.

Copy link
Member

Choose a reason for hiding this comment

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

Yep, it used to. However, we fixed that with #1889

That's just one problem we know about.

This was an outdated comment I saw by chance and accidentally never replied, so I thought it's a good idea to leave an answer here, but I don't see any need for action anymore.

This is a minor issue, but otherwise I disagree. We have to places in the official material that refer to the same directory for different purposes. They don't conflict now, but it's still an unnecessary source of confusion and potential problems, for the same reason you don't use one variable for two different purposes.

@wilzbach wilzbach deleted the fix-17649 branch December 18, 2017 06:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants