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

[install] fix Issue 11579 - dlang.org repo can't be built without git #422

Merged
merged 5 commits into from Dec 29, 2013

Conversation

MartinNowak
Copy link
Member

  • use github API for tags
  • use tarballs instead of git clone

@MartinNowak MartinNowak restored the fix11579 branch November 29, 2013 23:52
@MartinNowak
Copy link
Member Author

@andralex can you have a look at this? I need it to move on with automatic build scripts.

@MartinNowak
Copy link
Member Author

Someone else maybe? @braddr @9rnsr

@MartinNowak
Copy link
Member Author

Quick rationale:
The build/installer scripts already fetch sources which should be reused when building documentation.
With the git dependency in the makefile this is not easily possible.

@@ -167,7 +167,7 @@ clean:
rm -rf $(DOC_OUTPUT_DIR) ${LATEST}.ddoc
rm -rf auto dlangspec-tex.d $(addprefix dlangspec,.aux .d .dvi .fdb_latexmk .fls .log .out .pdf .tex)
rm -f docs.json docs-prerelease.json
@echo You should issue manually: rm -rf ${DMD_DIR}.${LATEST} ${DRUNTIME_DIR}.${LATEST} ${PHOBOS_DIR}.${LATEST}
@echo You should issue manually: rm -rf ${DMD_DIR}-${LATEST} ${DRUNTIME_DIR}-${LATEST} ${PHOBOS_DIR}-${LATEST}
Copy link
Member

Choose a reason for hiding this comment

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

What motivates this change?

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's the name of the tarball root directory (e.g. dmd-2.064.2). The same naming scheme is used/expected by rpmbuild and the deb packaging tool.

Copy link
Member

Choose a reason for hiding this comment

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

OK, fine with this

@andralex
Copy link
Member

I'm not convinced about the rationale. So we have one seemingly unexplained change of naming from . to -, and we switch from requiring git to requiring curl. I don't see progress here.

Why do we need to support machines without git?

Why don't the build/installer scripts use git as well, and if they don't, why can't we make them to? Using git seems like the logical choice instead of adding yet another moving piece - the historical archives.

Without very solid motivation, I am opposed to this. Sorry.

@MartinNowak
Copy link
Member Author

Why do we need to support machines without git?

Why don't the build/installer scripts use git as well, and if they don't, why can't we make them to?

Package build scripts are taball centric, for example the rpmbuild tool saves the tarballs in a source rpm (.srpm) file and builds the rpm package out of that. I'd need to add another script that creates tarballs from a temporary git clone, so directly downloading tarballs from github is cleaner.
Now that I already have all the sources I want to build the documentation from them which isn't possible because the folders are named differently and because posix.mak assumes git repos.

we switch from requiring git to requiring curl.

But curl is installed almost everywhere by default.

As a compromise the following would work for me.

  • Rename the folders to use a hyphen.
  • Only run git checkout if a .git subdirectory exists.

Another less important point, tarballs are smaller and faster to fetch than a complete git repo.

@andralex
Copy link
Member

How about we go with the compromise you propose, and consider a more involved solution for later? Thanks!

@MartinNowak
Copy link
Member Author

How about we go with the compromise you propose, and consider a more involved solution for later? Thanks!

Done.

[ -d $@ ] || git clone ${GIT_HOME}/$* $@/

# LATEST
${DMD_DIR}-${LATEST} ${DRUNTIME_DIR}-${LATEST} ${PHOBOS_DIR}-${LATEST} : ../%-${LATEST} :
Copy link
Member

Choose a reason for hiding this comment

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

Rules with multiple targets are almost never doing the right thing. In this case they're liable to execute the same commands multiple times. I see you have guards to prevent disasters, but I think there are better ways out there, see http://goo.gl/Qi6Qm8

Copy link
Member

Choose a reason for hiding this comment

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

Also, I've tried many times to make rules depending on directories, with bizarre results (the times when directories are written are... somewhat unpredictable). There's some order only prerequisite that may help: http://www.gnu.org/software/make/manual/html_node/Prerequisite-Types.html. Simpler, just plant a file .directory or .made to signal an action pertaining to a specific directory has succeeded.

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

There's some order only prerequisite that may help: http://www.gnu.org/software/make/manual/html_node/Prerequisite-Types.html.

Didn't knew this, but I went with .clone for now.

I'm not sure I understand what the issue with static pattern rules is.
Is it that because the rule is "generic", it may easier lead to mistakes if $@ et.al. don't get used?
What's the alternative, copying the rule for each target?

@andralex
Copy link
Member

TL;DR: I think multiple targets should be avoided.

@MartinNowak
Copy link
Member Author

updated

${MAKE} --directory=${DMD_DIR}.${LATEST}/src -f posix.mak clean
${MAKE} --directory=${DMD_DIR}.${LATEST}/src -f posix.mak -j 4
# HEAD
${DMD_DIR}/.clone ${DRUNTIME_DIR}/.clone ${PHOBOS_DIR}/.clone : ../%/.clone :
Copy link
Member

Choose a reason for hiding this comment

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

How does this pattern rule work? After macro expansion the whole thing is something like:

../dmd/.clone : ../%/.clone
    [ -d $(@D) ] || git clone ${GIT_HOME}/$* $(@D)/
    touch $@
../druntime/.clone : ../%/.clone
    [ -d $(@D) ] || git clone ${GIT_HOME}/$* $(@D)/
    touch $@
../phobos/.clone : ../%/.clone
    [ -d $(@D) ] || git clone ${GIT_HOME}/$* $(@D)/
    touch $@

According to http://www.gnu.org/software/make/manual/html_node/Pattern-Rules.html, the % must appear on the target side too. What is the intent here?

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 works like a normal pattern rule,

../%/.clone :
    [ -d $(@D) ] || git clone ${GIT_HOME}/$* $(@D)/
    touch $@

But it will only match the targets listed before the first colon.

${DMD_DIR}/.clone ${DRUNTIME_DIR}/.clone ${PHOBOS_DIR}/.clone

We could simply go with ../%/.clone : if that's OK for you.

Copy link
Member

Choose a reason for hiding this comment

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

The fewer subtleties the better. I'd say something like this is the ticket:

../%/.git-cloned :
    [ ! -e $(@D) ] || git clone ${GIT_HOME}/$* $(@D)/
    touch $@

Copy link
Member

Choose a reason for hiding this comment

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

Prolly even simpler:

../%/.git :
    [ ! -e $(@D) ] || git clone ${GIT_HOME}/$* $(@D)/

This counts on the cloning creating the .git directory.

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 used ../%/.cloned, after all not depending on git is the main purpose of this pull request.

.${LATEST} => -${LATEST}

- This matches the versioning scheme used by others
  (github, rpm spec, ...) and simplifies interop.
- check for existence of .git subdir first
@MartinNowak
Copy link
Member Author

updated

${MAKE} --directory=${DMD_DIR}.${LATEST}/src -f posix.mak -j 4
# HEAD
../%/.checkout :
[ -d $(@D) ] || git clone ${GIT_HOME}/$* $(@D)/
Copy link
Member

Choose a reason for hiding this comment

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

would we care to git pull if the directory does exist?

Copy link
Member Author

Choose a reason for hiding this comment

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

Only for a clean master maybe, anything else could have unwanted effect.
I don't think it's a good idea.

Copy link
Member

Choose a reason for hiding this comment

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

Then the name of the witness is confusing, should be .cloned or .directory - sorry, I know you've been there before and it's exasperating. But if I see .checkout I think of a witness for the last checkout.

Copy link
Member Author

Choose a reason for hiding this comment

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

done

andralex added a commit that referenced this pull request Dec 29, 2013
[install] fix Issue 11579 - dlang.org repo can't be built without git
@andralex andralex merged commit 8a6de6e into dlang:master Dec 29, 2013
@MartinNowak MartinNowak deleted the fix11579 branch December 29, 2013 21:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants