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

Add extra-html-files, for installing extra html files #1182

Merged
merged 6 commits into from Feb 6, 2013

Conversation

@jwiegley
Copy link

jwiegley commented Jan 14, 2013

For example, you might have an images/ directory in your project, with
images that you refer to from Haddock with:

<<images/foo.png>>

Then in your Cabal file you would include:

extra-html-files: images/*.png

And these would both be packaged by sdist, and "cabal haddock" will
install them in:

~/.cabal/share/doc/PROJECT/html/images/*.png

Fixes #1167

For example, you might have an images/ directory in your project, with
images that you refer to from Haddock with:

    <<images/foo.png>>

Then in your Cabal file you would include:

    extra-html-files: images/*.png

And these would both be packaged by sdist, and "cabal haddock" will
install them in:

    ~/.cabal/share/doc/PROJECT/html/images/*.png

Fixes #1167
@@ -223,7 +225,7 @@ prepareTree verbosity pkg_descr0 mb_lbi distPref targetDir pps = do

when (not (null (licenseFile pkg_descr))) $
copyFileTo verbosity targetDir (licenseFile pkg_descr)
flip mapM_ (extraSrcFiles pkg_descr) $ \ fpath -> do
flip mapM_ (extraSrcFiles pkg_descr ++ extraHtmlFiles pkg_descr) $ \ fpath -> do

This comment has been minimized.

Copy link
@23Skidoo

23Skidoo Jan 14, 2013

Member

forM_ maybe?

This comment has been minimized.

Copy link
@jwiegley

jwiegley Jan 14, 2013

Author

I certainly think forM_ is better there too, I just tried to keep the change as minimal as possible.

@23Skidoo

This comment has been minimized.

Copy link
Member

23Skidoo commented Jan 14, 2013

Can you also update the users' guide?

let dir = takeDirectory (flat_htmldir </> "html")
InstallDirs { htmldir = flat_htmldir } =
absoluteInstallDirs pkg_descr lbi NoCopyDest
-- sequence_ [ do copyFileTo verbosity dir file | file <- files ]

This comment has been minimized.

Copy link
@23Skidoo

23Skidoo Jan 14, 2013

Member

Do you need this comment?

This comment has been minimized.

Copy link
@jwiegley

jwiegley Jan 14, 2013

Author

No, I only put it in there because it matches how similar constructions are done elsewhere in the code, so I thought the project author may prefer that version for consistency. I think the forM_ usage is clearer, but sometimes people prefer uniformity everywhere.

@jwiegley

This comment has been minimized.

Copy link
Author

jwiegley commented Jan 14, 2013

I'll update the user's guide too, remove the comment, and change the flip mapM_.

@jwiegley

This comment has been minimized.

Copy link
Author

jwiegley commented Jan 14, 2013

Updated.

@23Skidoo

This comment has been minimized.

Copy link
Member

23Skidoo commented Jan 16, 2013

I did a quick test, and it seems to work incorrectly: cabal haddock builds the documentation in ./dist/doc, but extra-html-files are copied to ~/.cabal/share/doc. I'd expect my ~/.cabal dir to be updated only when I do a cabal install with --enable-documentation.

@@ -219,6 +220,13 @@ haddock pkg_descr lbi suffixes flags = do
lbi isVersion2 bi (commonArgs `mappend` exeArgs)
runHaddock verbosity keepTempFiles confHaddock exeArgs'
_ -> return ()

flip mapM_ (extraHtmlFiles pkg_descr) $ \ fpath -> do

This comment has been minimized.

Copy link
@23Skidoo

23Skidoo Jan 16, 2013

Member

Looks like you forgot this one.

@jwiegley

This comment has been minimized.

Copy link
Author

jwiegley commented Jan 16, 2013

I didn't really want to get into making extraneous code revisions, but I've changed a few more flip mapM_ to forM_. However, this is getting into changes completely unrelated to my extension.

@23Skidoo

This comment has been minimized.

Copy link
Member

23Skidoo commented Jan 17, 2013

What about my previous comment?

@jwiegley

This comment has been minimized.

Copy link
Author

jwiegley commented Jan 17, 2013

Ah, somehow I missed that one. I'll get that fixed.

@jwiegley

This comment has been minimized.

Copy link
Author

jwiegley commented Jan 23, 2013

Ok, it is now fix and uses the proper mechanism for installing locally and into ~/.cabal. The new code is actually quite a bit simpler than before, which is a good sign.

@23Skidoo

This comment has been minimized.

Copy link
Member

23Skidoo commented Jan 29, 2013

Sorry for the delay, I was travelling and had no Internet access. I will take a look at the patch soon.

@jwiegley

This comment has been minimized.

Copy link
Author

jwiegley commented Jan 30, 2013

@23Skidoo Thanks, for the update! I'm hoping this can make it into 1.17, as I know a few people who have been wanting something similar to this.

@byorgey

This comment has been minimized.

Copy link
Member

byorgey commented Jan 30, 2013

+1 for getting this merged and released soon!

@23Skidoo

This comment has been minimized.

Copy link

23Skidoo commented on Cabal/Distribution/Simple/Haddock.hs in a9bf1df Feb 3, 2013

Are you sure that you shouldn't be using unDir instead of unDir' here?

@23Skidoo

This comment has been minimized.

Copy link
Member

23Skidoo commented Feb 4, 2013

I've now tested it and it seems to work. Code also looks good to me.

Minor issue: if I set extra-html-files to something like doc/image.png, the file will be copied under ./dist/doc/html/$LIBNAME/doc/image.png or ~/.cabal/share/doc/$LIBNAME/html/doc/image.png (notice the additional doc). Is this the intended behaviour?

@tibbe, @dcoutts - I think this is ready to be merged, are you OK with the naming (extra-html-files)?

@jwiegley

This comment has been minimized.

Copy link
Author

jwiegley commented Feb 4, 2013

Yes, that was the intended behavior, since otherwise we have to deal with complicated rules about prefix stripping. Whatever the file's relative path within their project, is the relative path they should use to refer to the image within their Haddock link. That makes it all pretty straightforward.

@jwiegley

This comment has been minimized.

Copy link
Owner

jwiegley commented on a9bf1df Feb 4, 2013

I'm not sure at all, I am new to this code. Please make whatever change you feel is appropriate!

@23Skidoo

This comment has been minimized.

Copy link
Member

23Skidoo commented Feb 4, 2013

I'm going to merge this in if no-one protests.

@23Skidoo

This comment has been minimized.

Copy link
Member

23Skidoo commented Feb 6, 2013

@tibbe @dcoutts
I'm merging this in. Please revert if you disagree.

@jwiegley
Thanks again for the patch and for your patience during the review process.

23Skidoo added a commit that referenced this pull request Feb 6, 2013
Add extra-html-files, for installing extra html files
@23Skidoo 23Skidoo merged commit bb826d4 into haskell:master Feb 6, 2013
@jwiegley jwiegley deleted the jwiegley:extra-html-files branch Feb 6, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.