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

Use file instead of dir locking #187 #203

Merged
merged 2 commits into from
Feb 14, 2018
Merged

Conversation

snoyberg
Copy link
Contributor

This commit simply imports the code from the filelock package verbatim into a subdirectory, filelock. Depending on filelock as an external package instead would be more straightforward, but I'm not sure what the rules for external dependencies are here.

This commit simply imports the code from the filelock package verbatim
into a subdirectory, filelock. Depending on filelock as an external
package instead would be more straightforward, but I'm not sure what the
rules for external dependencies are here.
@snoyberg
Copy link
Contributor Author

Looks like this would address #126 as well.

@hvr
Copy link
Member

hvr commented Feb 13, 2018

...if we wanted to use the filelock package, we would simply depend on it... I already mentioned in another ticket I was working on this, and implied that filelock doesn't cut it.

@snoyberg
Copy link
Contributor Author

Unless you edited a comment to say something different than it did originally, I haven't seen any comment stating that filelock was considered and excluded, much less a reason given for this decision. I'd appreciate if you could point to where this comment was made.

Given that the extra dependency doesn't seem to be a problem, remove the
inlined code. If in fact the dependency should be avoided, just ignore
this commit and use the parent.
snoyberg added a commit to commercialhaskell/stack that referenced this pull request Feb 14, 2018
NOTE: This is included via an extra-dep, which would constitute the
first time Stack would include a patched version of an upstream library.
This is due to the fact that
haskell/hackage-security#203 is likely not going
to be merged, despite fixing issues affecting Stack. This leaves us with
(AFAICT) 4 choices at the Stack level:

1. Continue using the officially released upstream version of
   hackage-security, bugs and all
2. Fork hackage-security on Hackage, and depend on the fork
3. Inline the code from hackage-security into Stack itself, and drop the
   explicit dependency on hackage-security
4. Include hackage-security via an `extra-dep` pointing at a Git commit.
   Our official builds will use the patched version of hackage-security,
   and anyone building from Hackage will end up with the unpatched version

This PR represents approach (4). If and when the PR is merged and
released to Hackage, this becomes a non-issue. But generally speaking,
we should have a policy in Stack for handling these kinds of upstream
issues cases.
@23Skidoo
Copy link
Member

23Skidoo commented Feb 14, 2018

Even if a better solution is coming, can't we merge this PR for now as a stopgap and then drop the filelock dep from the next release once the better solution has been implemented?

@dcoutts
Copy link
Contributor

dcoutts commented Feb 14, 2018

@hvr I didn't see what the problems with filelock are? Can you point it out? On the face of it I'm inclined to agree that we take the patch and use filelock, and we can of course still make further changes and improvements later.

Copy link
Contributor

@dcoutts dcoutts left a comment

Choose a reason for hiding this comment

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

Code changes themselves look fine.

Still to resolve: do we use the filepath package directly or not (it only depends on base, unix & win32), and are there any problems with the filelock package.

@hvr hvr merged commit d91afd3 into haskell:master Feb 14, 2018
@hvr
Copy link
Member

hvr commented Feb 14, 2018

Fair enough, let's merge this as a a temporary stopgap solution for now.

@snoyberg snoyberg deleted the 187-file-locking branch February 14, 2018 11:14
@dcoutts
Copy link
Contributor

dcoutts commented Feb 14, 2018

Ok, so the issue is the unix flock() vs fnctl(). Locking semantics and impls on Unix are a mess. http://0pointer.de/blog/projects/locking.html

fnctl() is more portable but has pretty funky semantics, whereas flock is less portable but has saner semantics. It's also worth noting that the base 4.10 and above has partial support for file locking on windows and unix. We implemented this for the locking support in ghc & ghc-pkg of the package db. On unix the Handle locking is via flock, but it explicitly throws FileLockingNotImplemented for platforms that do not support flock so one has to pick a fallback in those cases.

snoyberg added a commit to commercialhaskell/stack that referenced this pull request Feb 14, 2018
NOTE: This is included via an extra-dep, which would constitute the
first time Stack would include a patched version of an upstream library.
This is due to the fact that
haskell/hackage-security#203 is likely not going
to be merged, despite fixing issues affecting Stack. This leaves us with
(AFAICT) 4 choices at the Stack level:

1. Continue using the officially released upstream version of
   hackage-security, bugs and all
2. Fork hackage-security on Hackage, and depend on the fork
3. Inline the code from hackage-security into Stack itself, and drop the
   explicit dependency on hackage-security
4. Include hackage-security via an `extra-dep` pointing at a Git commit.
   Our official builds will use the patched version of hackage-security,
   and anyone building from Hackage will end up with the unpatched version

This PR represents approach (4). If and when the PR is merged and
released to Hackage, this becomes a non-issue. But generally speaking,
we should have a policy in Stack for handling these kinds of upstream
issues cases.
@dcoutts
Copy link
Contributor

dcoutts commented Feb 14, 2018

@snoyberg thanks for poking through the issues that this resolves.

phadej added a commit to phadej/hackage-security that referenced this pull request Feb 14, 2018
snoyberg added a commit to commercialhaskell/stack that referenced this pull request Feb 16, 2018
NOTE: This is included via an extra-dep, which would constitute the
first time Stack would include a patched version of an upstream library.
This is due to the fact that
haskell/hackage-security#203 is likely not going
to be merged, despite fixing issues affecting Stack. This leaves us with
(AFAICT) 4 choices at the Stack level:

1. Continue using the officially released upstream version of
   hackage-security, bugs and all
2. Fork hackage-security on Hackage, and depend on the fork
3. Inline the code from hackage-security into Stack itself, and drop the
   explicit dependency on hackage-security
4. Include hackage-security via an `extra-dep` pointing at a Git commit.
   Our official builds will use the patched version of hackage-security,
   and anyone building from Hackage will end up with the unpatched version

This PR represents approach (4). If and when the PR is merged and
released to Hackage, this becomes a non-issue. But generally speaking,
we should have a policy in Stack for handling these kinds of upstream
issues cases.
@snoyberg
Copy link
Contributor Author

No problem @dcoutts :)

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.

4 participants