Skip to content

Checkout stable repositories into .generated #1889

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 2 commits into from
Dec 19, 2017

Conversation

wilzbach
Copy link
Member

@wilzbach wilzbach commented Sep 9, 2017

My main D directory got spammed with:

dmd
dmd-2.072.0
dmd-2.073.0
dmd-2.074.0
....
phobos-2.075.0

Thus, as dlang.org only needs those repositories, it would be very nice to keep things clean and put every into $(GENERATED)
Also currently the dmd-$(VERSION) folders conflict with the archives downloaded by the DLang installer.

@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.

@wilzbach wilzbach force-pushed the move-versions-to-generated branch from b3eee6c to 23d525f Compare September 10, 2017 01:27
@PetarKirov
Copy link
Member

I'm very much in favor of moving all the downloading to .generated/, the only problem remaining is that we also need to keep the ability to specify existing dmd, druntime and phobos for the case where you want to checkout your personal branch of either of those projects.

@wilzbach
Copy link
Member Author

CC @CyberShadow @ZombineDev - this is finally green 🎉
It was failing because the test target which is checking for trailing whitespace in ddoc files correctly found trailing whitespaces in https://github.com/dlang/dmd/blob/v2.076.0/changelog/staticforeach.dd (i.e. in .generated/dmd-2.076.0)
As we can't change this fact for now, I opted to ignore .generated for this check temporarily. In the future we should probably extend the check to all D repos, s.t. such things don't sneak in...

@CyberShadow
Copy link
Member

CyberShadow commented Sep 14, 2017

Some thoughts:

  • git clean will skip over repositories, so we need to be careful to not leave stale files inside these repositories. IIRC we ran into this with Digger/DAutoTest where a build was succeeding only because there was a successfully-built file hidden in such a subrepository. With a separate repository it's at least clear that there is some external state not being cleaned up by anything, here git clean may only appear to succeed.
  • Depending on how things are done on the user side, placing them in .generated means they may get cloned more often (pointless work and a break-prone task, depending on network connectivity). Is wiping .generated part of the clean target?

Overall I'm not entirely convinced this is an improvement, as it looks like trading off tidiness with some potential pitfalls.

Also currently the dmd-$(VERSION) folders conflict with the archives downloaded by the DLang installer.

How would they conflict? Does the installer unpack things to some parent directory of itself?

@wilzbach
Copy link
Member Author

wilzbach commented Oct 5, 2017

How would they conflict? Does the installer unpack things to some parent directory of itself?

Yes the installer installs the dmd into ~/dlang/dmd-$(VERSION) - if you happen to use have ~/dlang/dlang.org they both conflict: ~/dlang/dmd-2.074.0 could either by the release bundle OR a git clone from dlang/dmd.

Is wiping .generated part of the clean target?

Yes.

git clean will skip over repositories, so we need to be careful to not leave stale files inside these repositories.

Well but don't you run the clean Makefile in the beginning of DAutoTest?

Overall I'm not entirely convinced this is an improvement, as it looks like trading off tidiness with some potential pitfalls.

Yes, an alternative would be to have a permanent setting file for the installer, s.t. a custom download path can be specified permanently. If I find time, I might have a look into this, until then I will keep the PR open as a reminder and maybe other arguments pop up ...

@CyberShadow
Copy link
Member

if you happen to use have ~/dlang/dlang.org they both conflict

And how would one happen to use have that?

Perhaps the installer should use ~/.cache OSLT...

Well but don't you run the clean Makefile in the beginning of DAutoTest?

We can do anything we want with DAutoTest, that's not a problem and not what I was talking about. However, it was a surprise for me when we started having .git repositories in DAutoTest, and it might be a surprise to others.

@wilzbach wilzbach force-pushed the move-versions-to-generated branch from f5447e0 to 7201242 Compare December 7, 2017 07:28
@wilzbach
Copy link
Member Author

wilzbach commented Dec 7, 2017

Perhaps the installer should use ~/.cache OSLT...

-> dlang/installer#274

However, it was a surprise for me when we started having .git repositories in DAutoTest, and it might be a surprise to others.

Yes, but we still checkout the git repos, it's just that now we clone them into .generated, s.t. it's neatly organized, doesn't clog the ~/dlang directory and gets removed on make -f posix.mak clean.
(I just rebased this)

@wilzbach
Copy link
Member Author

Ping @CyberShadow - this is finally passing. Can we move on with this or do you still have concerns?
It would really help to keep my ~/dlang directory clean and organized. I clean it every now and then, but it still adds up:

> ls ~/dlang | grep -E "(dmd|druntime|phobos)"
dmd/
dmd-2.062/
dmd-2.064/
dmd-2.065.0/
dmd-2.066.0/
dmd-2.067.0/
dmd-2.070.2/
dmd-2.071.1/
dmd-2.072.0/
dmd-2.072.2/
dmd-2.073.0/
dmd-2.073.1/
dmd-2.073.2/
dmd-2.074.0/
dmd-2.074.1/
dmd-2.075.0/
dmd-2.075.0-b2/
dmd-2.076.0/
dmd-2.076.1/
dmd-2.077.0/
dmd-2.077.1/
druntime/
druntime-2.073.1/
druntime-2.074.0/
druntime-2.074.1/
druntime-2.075.0/
druntime-2.076.0/
druntime-2.076.1/
druntime-2.077.0/
druntime-2.077.1/
phobos/
phobos-2.074.0/
phobos-2.074.1/
phobos-2.075.0/
phobos-2.076.0/
phobos-2.076.1/
phobos-2.077.0/
phobos-2.077.1/
``

(a couple of the DMD folders are from the installer, which as mentioned is even more annoying because it creates conflicts)

@CyberShadow
Copy link
Member

Well, I don't like it purely subjectively - I kind of enjoyed the stable version checkouts being where they were, and sometimes made use of them since they were already there. If other D components were to need stable checkouts, they will now not be reusable. I'm also a little sad about going away from avoiding pointless work - all the redundant cloning is just wasted time and network traffic.

So, I'm not sure I can judge this objectively. There is no immediate technical reason not to merge it, but I also don't see an immediate technically-motivated urgency in merging it either.

@PetarKirov
Copy link
Member

If other D components were to need stable checkouts, they will now not be reusable.

@CyberShadow You can still reuse them by setting them the *_LATEST_DIR makefile variables manually. Is this enough to cover your concerns about this PR?

@CyberShadow
Copy link
Member

Yes, go ahead and merge it if you agree with the change.

@@ -152,21 +152,21 @@ DPL_DOCS_PATH=dpl-docs
DPL_DOCS=$(DPL_DOCS_PATH)/dpl-docs
[email protected]:data
TMP?=/tmp
GENERATED=.generated
G=$(GENERATED)
Copy link
Member

Choose a reason for hiding this comment

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

Shall we just use G and delete GENERATED?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't have a strong opinion about this, having it like this allows people to overwrite the more expressive GENERATED variable and doesn't cost us too much.
Note that I just moved these two lines up, s.t. the variables are in order (i.e. the are defined before used).

If you or someone else has a stronger opinion about this, let me know and then I will make a PR ;-)

@wilzbach wilzbach force-pushed the move-versions-to-generated branch from bbddac2 to 2969c36 Compare December 19, 2017 07:26
@PetarKirov
Copy link
Member

@wilzbach the build failed downloading dmd 2.072.1 - you need to rebase this PR to restart it.

@dlang-bot dlang-bot merged commit 78e54e8 into dlang:master Dec 19, 2017
wilzbach added a commit to wilzbach/dlang.org that referenced this pull request Dec 19, 2017
@wilzbach wilzbach deleted the move-versions-to-generated branch December 19, 2017 07:58
@wilzbach
Copy link
Member Author

@ZombineDev thanks a lot! I realized that I made a small mistake and the repositories are currently checked out into the root folder -> #2016

dlang-bot added a commit that referenced this pull request Dec 19, 2017
Fixup for #1889: Use $G instead of ../$G
merged-on-behalf-of: Petar Kirov <[email protected]>
@MartinNowak
Copy link
Member

Fundamentally all of those latest release builds are a mess. We should build /spec, /phobos, and /library for a specific release once (actually with the release), upload it to a server and never touch them again.
Rebuilding the docs from the latest release with every change on master is fragile and modifying already released docs is not wanted.
https://docarchives.dlang.io is a good first step, but not fully embedded in dlang.org which does receive daily updates from master.

@wilzbach
Copy link
Member Author

Rebuilding the docs from the latest release with every change on master is fragile

But ensures that our documentation doesn't look too horrible.

modifying already released docs is not wanted.

Hmm, and I wanted to ask you whether we can build the docs from stable instead of the fixed release, because it often happens that it takes a month for a simple UI fix (typo, Ddoc bug, ...) to reach dlang.org

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.

6 participants