Skip to content

AppVeyor configuration #206

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 2 commits into from
Closed

AppVeyor configuration #206

wants to merge 2 commits into from

Conversation

@snoyberg
Copy link
Contributor

@snoyberg snoyberg commented Feb 14, 2018

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
@ezyang @snoyberg
Make hackage-repo-tool build on Windows.
b8e9798 
Fixes haskell#175. Signed-off-by: Edward Z. Yang <ezyang@cs.stanford.edu>
@snoyberg
Add AppVeyor configuration haskell#176
d7786db
@23Skidoo
Copy link
Member

23Skidoo commented Feb 14, 2018

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 •
edited
Loading

I wonder if it makes sense to support hackage-repo-tool on Windows?
@23Skidoo
Copy link
Member

23Skidoo commented Feb 14, 2018

The patch adding Windows support is not that big, so I think it's fine.
phadej
phadej reviewed Feb 14, 2018
View reviewed changes
hackage-repo-tool/src/Hackage/Security/RepoTool/Util/IO.hs
linkLoc' <- toAbsoluteFilePath linkLoc
Posix.createSymbolicLink linkTarget' linkLoc'
#else
error $ "Cannot create symbolic links on Windows"
Copy link
Contributor

@phadej phadej Feb 14, 2018

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

@23Skidoo 23Skidoo Feb 14, 2018

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

@phadej phadej Feb 14, 2018

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

@23Skidoo 23Skidoo Feb 14, 2018

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 •
edited
Loading

BTW, cabal-install's test suite uses hackage-repo-tool, so this could come in handy.
@23Skidoo 23Skidoo mentioned this pull request Feb 15, 2018
Closed
@snoyberg
Copy link
Contributor Author

snoyberg commented Feb 16, 2018

@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 •
edited
Loading

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 23Skidoo mentioned this pull request Feb 20, 2018
Merged
@23Skidoo
Copy link
Member

23Skidoo commented Feb 20, 2018

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

5 participants

@snoyberg @23Skidoo @phadej @hvr @ezyang