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

AppVeyor configuration #206

Closed
wants to merge 2 commits into from
Closed

Conversation

snoyberg
Copy link
Contributor

This includes the changes from #176 (CC @ezyang), rebased against master, and adds in an AppVeyor configuration as requested in #176. I've checked that the AppVeyor configuration works (see https://ci.appveyor.com/project/snoyberg/hackage-security/build/1.0.1), but someone will need to enable AppVeyor for haskell/hackage-security to make this work.

ezyang and others added 2 commits February 14, 2018 17:48
Fixes haskell#175.

Signed-off-by: Edward Z. Yang <ezyang@cs.stanford.edu>
@23Skidoo
Copy link
Member

I don't have admin access to this repo, so I can't enable AppVeyor. I think @hvr has.

@phadej
Copy link
Contributor

phadej commented Feb 14, 2018

I wonder if it makes sense to support hackage-repo-tool on Windows?

@23Skidoo
Copy link
Member

The patch adding Windows support is not that big, so I think it's fine.

createDirectoryIfMissing True (takeDirectory linkLoc)
linkTarget' <- toAbsoluteFilePath linkTarget
linkLoc' <- toAbsoluteFilePath linkLoc
Posix.createSymbolicLink linkTarget' linkLoc'
#else
error $ "Cannot create symbolic links on Windows"
Copy link
Contributor

Choose a reason for hiding this comment

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

@23Skidoo this makes symlink-cabal-local-repo command fail. Not that it's useful on Windows, but I'd much happier disabling that codepath cleanly (i.e. disabling the command on windows), and not creating technical debt.

Copy link
Member

Choose a reason for hiding this comment

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

Fine, but this #ifdef will still be needed.

Copy link
Contributor

Choose a reason for hiding this comment

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

by cleanly i mean: #ifdef the whole function, and see what breaks -> disable that with good error message.

Copy link
Member

Choose a reason for hiding this comment

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

OK, I can look into it tomorrow.

@23Skidoo
Copy link
Member

23Skidoo commented Feb 14, 2018

BTW, cabal-install's test suite uses hackage-repo-tool, so this could come in handy.

@snoyberg
Copy link
Contributor Author

@hvr I don't really mind if you want to go ahead and implement this yourself in a different way (23d8e91), but a comment in this PR explaining your motivations for posterity, and so that as a contributor I know what the status of my PR is, would be greatly appreciated.

@hvr
Copy link
Member

hvr commented Feb 16, 2018

It would be great if the review concerns from @phadej in Hackage.Security.RepoTool.Util.IO could be resolved and it'd also be nice if this PR could be rebased atop the new cabal-based appveyor script I implemented in master (it's optional though; I can take care of this myself when merging the PR); it should be enough to remove the

sed -i "s/hackage-repo-tool/--disabled/" cabal.project

line which currently disables hackage-repo-tool (as in master it doesn't have an install-plan for windows yet, and thus prevents the cabal project from being built at all) in the appveyor.yml script. If there happen to be any technical difficulties I'll be happy to take care of them.

@23Skidoo
Copy link
Member

Closing in favour of #210, which is a version of this PR with the points raised by @phadej and @hvr addressed.

@23Skidoo 23Skidoo closed this Feb 20, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants