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

Can we relax indir's test on the target directory? #5953

Closed
p6rt opened this issue Dec 30, 2016 · 18 comments
Closed

Can we relax indir's test on the target directory? #5953

p6rt opened this issue Dec 30, 2016 · 18 comments

Comments

@p6rt
Copy link

@p6rt p6rt commented Dec 30, 2016

Migrated from rt.perl.org#130460 (status was 'resolved')

Searchable as RT130460$

@p6rt
Copy link
Author

@p6rt p6rt commented Dec 30, 2016

From @briandfoy

indir is in S16 but not documented elsewhere, but it's a really cool
feature that I think many people will find useful. It's certainly much
more convenient

  indir '/Users/brian', {
  put "Directory is $*CWD";
  }

  indir '/etc', {
  put "Directory is $*CWD";
  }

The second one fails with

  Failed to change the working directory to '/etc'​: did not pass 'd r w' test

I don't know why it insists that the directory be writable, expecially
when chdir itself is not as strict. I often change to a directory where
I want to merely read files, so I'd like to see the test relaxed.

And, since its undocumented, nobody else knows that it does this currently. :)

@p6rt
Copy link
Author

@p6rt p6rt commented Dec 30, 2016

From @cognominal

Not only .indir default is strange, but the behavior of .chdir it relies
on is weird too.
I would like .chir :$test to support String|Code or array of thereof so as
to dwim

  :test<rw> # read/writeable by user
  :test<r w> # same
  :test< arw > # same but for all, patterned
after chmod
  :test< ---rx-rwx > # patterned after ls -a (one
stat(2))

Or is this overengineering ?

The current code in .chdir looks like a placeholder for something to come.

  if $test eq 'r' {
  return $dir if $dir.r;
  }
  elsif $test eq 'r w' {
  return $dir if $dir.r and $dir.w;
  }
  elsif $test eq 'r w x' {
  return $dir if $dir.r and $dir.w and $dir.x;
  }

On Fri, Dec 30, 2016 at 11​:06 PM, brian d foy <perl6-bugs-followup@​perl.org>
wrote​:

# New Ticket Created by "brian d foy"
# Please include the string​: [perl #​130460]
# in the subject line of all future correspondence about this issue.
# <URL​: https://rt-archive.perl.org/perl6/Ticket/Display.html?id=130460 >

indir is in S16 but not documented elsewhere, but it's a really cool
feature that I think many people will find useful. It's certainly much
more convenient

indir '/Users/brian', \{
    put "Directory is $\*CWD";
    \}

indir '/etc', \{
    put "Directory is $\*CWD";
    \}

The second one fails with

Failed to change the working directory to '/etc'&#8203;: did not pass 'd r w'

test

I don't know why it insists that the directory be writable, expecially
when chdir itself is not as strict. I often change to a directory where
I want to merely read files, so I'd like to see the test relaxed.

And, since its undocumented, nobody else knows that it does this
currently. :)

--
cognominal stef

@p6rt
Copy link
Author

@p6rt p6rt commented Dec 30, 2016

The RT System itself - Status changed from 'new' to 'open'

@p6rt
Copy link
Author

@p6rt p6rt commented Dec 30, 2016

From @zoffixznet

On Fri, 30 Dec 2016 14​:06​:51 -0800, comdog wrote​:

And, since its undocumented, nobody else knows that it does this
currently. :)

That's because there's not a single mention of it in the 6.c Perl 6 Language Specification[^1] and we don't document unspecced things.

Personally, I rather leave it that way for now, until we get a chance to revamp our IO API (which very likely won't be possible until 6.d language release). Passing those strings for $test is pretty inflexible and bug prone and I rather see us toss that sort of API from chdir; and the fewer things rely on its current behaviour the better.

[1] https://github.com/perl6/roast

@p6rt
Copy link
Author

@p6rt p6rt commented Dec 31, 2016

From @lizmat

FWIW, I think this is a leftover / half baked thing from the newio branch.

On 30 Dec 2016, at 23​:34, Stéphane Payrard <cognominal@​gmail.com> wrote​:

Not only .indir default is strange, but the behavior of .chdir it relies on is weird too.
I would like .chir :$test to support String|Code or array of thereof so as to dwim

:test<rw> # read/writeable by user
:test<r w> # same
:test< arw > # same but for all, patterned after chmod
:test< ---rx-rwx > # patterned after ls -a (one stat(2))

Or is this overengineering ?

The current code in .chdir looks like a placeholder for something to come.

 if $test eq 'r' \{
        return $dir if $dir\.r;
    \}
    elsif $test eq 'r w' \{
        return $dir if $dir\.r and $dir\.w;
    \}
    elsif $test eq 'r w x' \{
        return $dir if $dir\.r and $dir\.w and $dir\.x;
    \}

On Fri, Dec 30, 2016 at 11​:06 PM, brian d foy <perl6-bugs-followup@​perl.org> wrote​:
# New Ticket Created by "brian d foy"
# Please include the string​: [perl #​130460]
# in the subject line of all future correspondence about this issue.
# <URL​: https://rt-archive.perl.org/perl6/Ticket/Display.html?id=130460 >

indir is in S16 but not documented elsewhere, but it's a really cool
feature that I think many people will find useful. It's certainly much
more convenient

indir '/Users/brian', \{
    put "Directory is $\*CWD";
    \}

indir '/etc', \{
    put "Directory is $\*CWD";
    \}

The second one fails with

Failed to change the working directory to '/etc'&#8203;: did not pass 'd r w' test

I don't know why it insists that the directory be writable, expecially
when chdir itself is not as strict. I often change to a directory where
I want to merely read files, so I'd like to see the test relaxed.

And, since its undocumented, nobody else knows that it does this currently. :)

--
cognominal stef

@p6rt
Copy link
Author

@p6rt p6rt commented Jan 1, 2017

From @briandfoy

That's because there's not a single mention of it in the 6.c Perl 6
Language Specification[^1] and we don't document unspecced things.

That's contrary to my experience. For instance, https://rt-archive.perl.org/perl6/Ticket/Display.html?id=130419 , along with many other things I've found.

Since the spec is basically a test in roast, that's an easy technical fix.

But, you said "your PR testing or documenting it will be rejected" (https://www.learningperl6.com/2016/12/30/indir-changes-the-directory-just-for-its-block/#comment-348). If patches aren't welcome, then there's a different issue to address​: Is there a roadmap or other notes that pull together the direction for future work, what's in or out, what's off limits, and such.

@p6rt
Copy link
Author

@p6rt p6rt commented Jan 2, 2017

From @toolforger

Am 30.12.2016 um 23​:06 schrieb brian d foy (via RT)​:

I don't know why it insists that the directory be writable, expecially
when chdir itself is not as strict. I often change to a directory where
I want to merely read files, so I'd like to see the test relaxed.

If you go by Posix logic, indir and chdir need just searchability (the x
permission), i.e. the ability to access a file with a given name.

Readability (r) is for getting a list of all files.

Unless there is a reason for requiring readability for chdir or indir, I
think that neither should even do any additional checking; just let them
call the Posix function and see what error codes are returned.

The usual reason would be portability, but at least Windows follows the
Posix path in this regard.

@p6rt
Copy link
Author

@p6rt p6rt commented Jan 2, 2017

From @geekosaur

On Fri, Dec 30, 2016 at 5​:41 PM, Joachim Durchholz <jo@​durchholz.org> wrote​:

The usual reason would be portability, but at least Windows follows the
Posix path in this regard.

But all bets are off if it's a network filesystem.

--
brandon s allbery kf8nh sine nomine associates
allbery.b@​gmail.com ballbery@​sinenomine.net
unix, openafs, kerberos, infrastructure, xmonad http://sinenomine.net

@p6rt
Copy link
Author

@p6rt p6rt commented Jan 2, 2017

From @briandfoy

I would expect indir to call chdir and let the exceptions fall where
they may. If I indir into something I'm not allowed to do, let the
exception come later. Even if I can execute that directory, it doesn't
mean I can read the files in it, for instance.

To me, the spirit of Perl has been "if that's what you want to do,
here you go". I'd rather take on the burden of handling things if they
blow up for things like this if it's a choice between that and not
even getting to try it.

@p6rt
Copy link
Author

@p6rt p6rt commented Jan 2, 2017

From @toolforger

Am 02.01.2017 um 22​:28 schrieb brian d foy​:

To me, the spirit of Perl has been "if that's what you want to do,
here you go". I'd rather take on the burden of handling things if they
blow up for things like this if it's a choice between that and not
even getting to try it.

Um, well, if it were possible to really build a nicer interface on top
of chdir, then I'd prefer that. Keeps some more mistakes out of
application programmer code.
In the case of chdir/indir, I don't think it's possible to outsmart the
platform quirks, or second-guess the programmer's intentions.
So I come to the same conclusion but for a different set of reasons :-)

@p6rt
Copy link
Author

@p6rt p6rt commented Jan 3, 2017

From @zoffixznet

On Sun, 01 Jan 2017 10​:48​:07 -0800, comdog wrote​:

That's because there's not a single mention of it in the 6.c Perl 6
Language Specification[^1] and we don't document unspecced things.

That's contrary to my experience. For instance,
https://rt-archive.perl.org/perl6/Ticket/Display.html?id=130419 , along with many
other things I've found.

That's because many things were documented prior to our installment of that policy and have been simply missed.
I strongly recommend you join our IRC channel[1] and ask questions if you're unsure if something is
supposed to be documented/specced/implemented.

Since the spec is basically a test in roast, that's an easy technical
fix.

Well, it's true, it's very easy to lock us into supporting the current API and behaviour
by writing a simple test.

I see support from users for having the `indir` routine, so I doubt any change would involve removing it from
current codebase. However, its API and behaviour are quite non-ideal, which is why I commented
on your article saying that just documenting and testing the current API isn't what we want.

Is there a roadmap or other notes that pull together the direction for
future work, what's in or out, what's off limits, and such.

Nothing I have in writing yet, as I set off on this quest to improve IO when I went to fix your `chdir`
ticket and saw the horrors of the current code. You're asking because you want to include it in your book, yes?
Because in that case I can narrow down potential changes​:

1) We'll still have `indir` (so adding spectests today just for basic functionality, like we have for `chdir` is OK, I think)
2) It'll likely `fail` instead of `throw`ing as it does now
3) There's currently a race condition in it that makes it perform things in the wrong directory; unsure how/whether fixing it would affect the API
4) The topic of this ticket​: its default set of tests for the directory will change (just test if it's a directory?)
5) It most definitely won't be taking the `​:$test` parameter as some predefined string as it's bug prone and isn't consistent with the way we pass arguments around elsewhere. Most likely it'd be just adverbs, like we have on, say, .match. So instead of passing :test<r d x> (ensuring you get them in right order!), you'd pass the tests you want as :r, :w, :x adverbs.

As for roadmap, first I plan to make a "map" of all of our core routines to find out what calling convention is most common and what's the typical way to indicate failure. That will guide the decision for the calling form and failure mode for the `indir` and related IO routines.

In the past year, any time something awful about IO was brought up, the `newio` branch was mentioned as being one where the awfulness was fixed. It's a branch that was made prior to first Christmas release, but never got merged. So the next step on the roadmap would be to examine the changes[^2] that branch would've made were it merged before Christmas release and see what we can/want to have in current code, to improve our IO.

Not every change will be possible, because we can't break existing spectests, and that's precisely the reason why spectests for current API of indir should not be added.

[1] https://webchat.freenode.net/?channels=#perl6
[2] rakudo/rakudo@6bbb56f...newio

Cheers,
ZZ

@p6rt
Copy link
Author

@p6rt p6rt commented Jan 3, 2017

From @toolforger

Am 03.01.2017 um 01​:48 schrieb Zoffix Znet via RT​:

I see support from users for having the `indir` routine,

Yes, chdir is nasty because it's a global setting and you never know
which of the functions you're calling might be changing the setting.

indir solves all these things. (Provided it changes the current
directory back even in the face of exceptions.)

4) The topic of this ticket​: its default set of tests for the directory will change (just test if it's a directory?)

Some platform might even be okay with a nondirectory for chdir. This
wouldn't be particularly useful, but we can't assume that all platforms
do only useful things.

That said, I think the indir tests should just test that chdir is being
called at the right times with the right arguments (can Perl6 tests do
mocking? that would be needed)
So any error case testing can be done for chdir.

Not every change will be possible, because we can't break existing spectests,

Oh, is Perl6 that stable now?
I thought it should still be possible to fix mistakes.

and that's precisely the reason why spectests for current API of
indir should not be added.

+1

@p6rt
Copy link
Author

@p6rt p6rt commented Jan 3, 2017

From @lizmat

On 3 Jan 2017, at 06​:58, Joachim Durchholz <jo@​durchholz.org> wrote​:
Yes, chdir is nasty because it's a global setting and you never know which of the functions you're calling might be changing the setting.

indir solves all these things. (Provided it changes the current directory back even in the face of exceptions.)

Please note that there is no real chdir() at the OS level, because that is a bad idea in a threaded environment. And for that reason, the JVM doesn’t allow for it, afaik, and thus it wouldn’t work on the JVM backend anyway.

The “current directory” is represented by the $*CWD dynamic variable. Changing the “current directory” in a scope, is nothing else than syntactic sugar around creating a local lexical $*CWD. By virtue of the destruction of lexical variables when they go out of scope, this will also work if the scope is exited by an exception.

4) The topic of this ticket​: its default set of tests for the directory will change (just test if it's a directory?)
Some platform might even be okay with a nondirectory for chdir. This wouldn't be particularly useful, but we can't assume that all platforms do only useful things.

That said, I think the indir tests should just test that chdir is being called at the right times with the right arguments (can Perl6 tests do mocking? that would be needed)
So any error case testing can be done for chdir.

Since changing the current directory is nothing else than changing $*CWD, mocking should not be a problem.

Not every change will be possible, because we can't break existing spectests,
Oh, is Perl6 that stable now?
I thought it should still be possible to fix mistakes.

I hear they’re still fixing mistakes in Perl 5. Which, in my impression, is considered stable by many :-)

Liz

@p6rt
Copy link
Author

@p6rt p6rt commented Jan 3, 2017

From @geekosaur

On Tue, Jan 3, 2017 at 12​:58 AM, Joachim Durchholz <jo@​durchholz.org> wrote​:

indir solves all these things. (Provided it changes the current directory
back even in the face of exceptions.)

Edge case​: current directory can't be re-entered by path (as with the old
MMDF trick​: daemon launcher running as root chdir()s to a directory via a
path where one component is only searchable by root, then drops perms and
runs the real daemon. Another way to do it is rename() or rmdir() one path
component). This can be worked around if the platform supports fchdir(),
otherwise you just can't get back where you were.

--
brandon s allbery kf8nh sine nomine associates
allbery.b@​gmail.com ballbery@​sinenomine.net
unix, openafs, kerberos, infrastructure, xmonad http://sinenomine.net

@p6rt
Copy link
Author

@p6rt p6rt commented Jan 3, 2017

From @toolforger

Am 03.01.2017 um 09​:59 schrieb Elizabeth Mattijsen​:

On 3 Jan 2017, at 06​:58, Joachim Durchholz <jo@​durchholz.org> wrote​:
Yes, chdir is nasty because it's a global setting and you never know which of the functions you're calling might be changing the setting.

indir solves all these things. (Provided it changes the current directory back even in the face of exceptions.)

Please note that there is no real chdir() at the OS level, because that is a bad idea in a threaded environment. And for that reason, the JVM doesn’t allow for it, afaik, and thus it wouldn’t work on the JVM backend anyway.

That would be the JDK, not the JVM (which does not implement these things).
But right, there is no good way to change the current working directory
in Java, which is the Right Thing to Do(TM) in my book actually.

The “current directory” is represented by the $*CWD dynamic variable. Changing the “current directory” in a scope, is nothing else than syntactic sugar around creating a local lexical $*CWD. By virtue of the destruction of lexical variables when they go out of scope, this will also work if the scope is exited by an exception.

Ah, I didn't know what.
That's much better than what Python has :-)

4) The topic of this ticket​: its default set of tests for the directory will change (just test if it's a directory?)
Some platform might even be okay with a nondirectory for chdir. This wouldn't be particularly useful, but we can't assume that all platforms do only useful things.

That said, I think the indir tests should just test that chdir is being called at the right times with the right arguments (can Perl6 tests do mocking? that would be needed)
So any error case testing can be done for chdir.

Since changing the current directory is nothing else than changing $*CWD, mocking should not be a problem.

+1

Not every change will be possible, because we can't break existing spectests,
Oh, is Perl6 that stable now?
I thought it should still be possible to fix mistakes.

I hear they’re still fixing mistakes in Perl 5. Which, in my impression, is considered stable by many :-)

:-D

@p6rt
Copy link
Author

@p6rt p6rt commented Jan 29, 2017

From aar@cpan.org

On Tue, 03 Jan 2017 01​:00​:52 -0800, elizabeth wrote​:

The “current directory” is represented by the $*CWD dynamic variable.
Changing the “current directory” in a scope, is nothing else than
syntactic sugar around creating a local lexical $*CWD.

In fact, I don't see why would indir perform additional checks and/or have any different behavior than​:

  temp $*CWD = chdir($dir);

or even​:

  temp $*CWD = $dir;

(Do we actually need that much syntactic sugar in the core, for things that are clear enough and not that verbose? The "foo $tempvalue, BLOCK" pattern could be an alternative for "temp" in so many cases... are we going to cover them all?)

@p6rt
Copy link
Author

@p6rt p6rt commented Apr 3, 2017

From @zoffixznet

On Fri, 30 Dec 2016 14​:06​:51 -0800, comdog wrote​:

indir is in S16 but not documented elsewhere, but it's a really cool
feature that I think many people will find useful. It's certainly much
more convenient

indir '/Users/brian', {
put "Directory is $*CWD";
}

indir '/etc', {
put "Directory is $*CWD";
}

The second one fails with

Failed to change the working directory to '/etc'​: did not pass 'd r w'
test

I don't know why it insists that the directory be writable, expecially
when chdir itself is not as strict. I often change to a directory
where
I want to merely read files, so I'd like to see the test relaxed.

And, since its undocumented, nobody else knows that it does this
currently. :)

Thank you for the report. This ticket is now resolved.

`indir` is now part of the Perl 6 language. Please consult with the documentation for its behaviour​:
https://docs.perl6.org/routine/indir

@p6rt p6rt closed this Apr 3, 2017
@p6rt
Copy link
Author

@p6rt p6rt commented Apr 3, 2017

@zoffixznet - Status changed from 'open' to 'resolved'

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
1 participant