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

Support shopt -s dotglob #2194

Merged
merged 2 commits into from
Dec 13, 2024
Merged

Support shopt -s dotglob #2194

merged 2 commits into from
Dec 13, 2024

Conversation

leungbk
Copy link
Collaborator

@leungbk leungbk commented Dec 11, 2024

Related to #610.

The Python build seems okay, but I could use some help getting the ninja build to pass; my attempt at defining GLOB_PERIOD doesn't seem to work.

In file included from /home/brian/oils/cpp/preamble.h:6:
_gen/bin/oils_for_unix.mycpp.cc: In member function ‘int glob_::Globber::_Glob(BigStr*, List<BigStr*>*)’:
_gen/bin/oils_for_unix.mycpp.cc:41233:22: error: expected unqualified-id before ‘(’ token
41233 |       flags |= libc::GLOB_PERIOD;
      |                      ^~~~~~~~~~~

@leungbk leungbk marked this pull request as draft December 11, 2024 20:14
@leungbk leungbk marked this pull request as ready for review December 11, 2024 20:42
@leungbk
Copy link
Collaborator Author

leungbk commented Dec 11, 2024

OK, I managed to resolve my problem from before: mycpp/TEST.sh test-translator now passes on my machine.

But I cannot tell from the GitHub interface where specifically to look to investigate the failing jobs; clicking on any single job takes me to a publish-html step that doesn't appear to expose anything helpful.

@andychu
Copy link
Contributor

andychu commented Dec 11, 2024

Cool, I will look at this!

Oh there is a URL hidden in the logs.

https://github.com/oils-for-unix/oils/actions/runs/12284550355/job/34280567011?pr=2194

I switched from SSH to HTTP copying, and it introduced a lot of spew, which has hidden it

I can fix that


In the mean time I go to

https://op.oilshell.org/uuu/github-jobs/

which is linked on https://www.oilshell.org/ and then look for the CI results


Thanks for the help!

@andychu
Copy link
Contributor

andychu commented Dec 11, 2024

Oh actually you can see a few more failures here

https://op.oilshell.org/uuu/github-jobs/8712/

https://op.oilshell.org/uuu/github-jobs/8712/dev-setup-debian.wwz/_tmp/soil/logs/py-all-and-ninja.txt

.......
======================================================================
ERROR: testGlob (__main__.LibcTest)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "pyext/libc_test.py", line 177, in testGlob
    print(libc.glob('\\'))
RuntimeError: unknown problem

----------------------------------------------------------------------
Ran 12 tests in 0.001s

FAILED (errors=1)

I am not sure what happened, I guess the default behavior changed somehow?

@leungbk leungbk force-pushed the leungbk-dotglob branch 4 times, most recently from 10bd6ee to 5e1fc27 Compare December 12, 2024 00:34
@leungbk
Copy link
Collaborator Author

leungbk commented Dec 12, 2024

Thanks, I managed to fix those problems. I have some questions, though.

First, I am not sure how to properly declare the libc.glob wrapper to have the second argument as optional; in spite of my best efforts, I found that I needed to do stuff like

@@ -45,12 +45,12 @@ class LibcTest(unittest.TestCase):
 
   def testGlob(self):
     print('GLOB')
-    print(libc.glob('*.py'))
+    print(libc.glob('*.py', 0))
 
     # This will not match anything!
-    print(libc.glob('\\'))
+    print(libc.glob('\\', 0))
     # This one will match a file named \
-    print(libc.glob('\\\\'))
+    print(libc.glob('\\\\', 0))
 
   def testRegex(self):
     #print(libc.regcomp(r'.*\.py'))

Finally, one of the previously-failing tests in globignore.test.sh is now improperly passing due to this change I made:

            # NOTE: libc's glob function can return '.' and '..',
            # which are typically not of interest. Filtering in
            # this manner is similar (possibly identical?) to the
            # default bash setting of 'setopt -s
            # globskipdots'. However, this does not appear to
            # faithfully reproduce the globskipdots' OFF setting,
            # so supporting that option fully would require more
            # than simply wrapping this in an if statement.
            tmp = [s for s in results if s not in ['.', '..']]
            results = tmp
            n = len(results)

Is it OK to have that test pass in this manner? It is complete happenstance: we support neither of the two features (globskipdots and globignore) exercised by that test, but since my change to the glob function filtered out . and .., that test started to pass.

@andychu
Copy link
Contributor

andychu commented Dec 12, 2024

Wow this looks great, thank you! Thanks for tracking down all the Python and C++ stuff

I think it can be OK to just pass 0 everywhere for the glob() flags ... I don't think we have any optional args in Python right now.


If we don't support globskipdots, then maybe we can tweak the test so it doesn't pass? I guess we are missing test coverage?

@leungbk
Copy link
Collaborator Author

leungbk commented Dec 12, 2024

OK, I have updated the globstar test to make sure it continues to fail.

I also corrected my comment in glob_.py: filtering out only . and .. in the manner that I did does not always match the behavior of shopt -s globskipdots. I tried to look at how bash implemented it but couldn't quite follow all the casework.

Copy link
Contributor

@andychu andychu left a comment

Choose a reason for hiding this comment

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

Thanks for doing this!

osh/glob_.py Outdated
# setting of 'setopt -s globskipdots'. Supporting that
# option fully would require more than simply wrapping
# this in an if statement.
tmp = [s for s in results if s not in ['.', '..']]
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's fair not to support globskipdots now ... I didn't even know that existed.


I think this change is ready because it makes more tests pass.

But it may be a good idea to rewrite it so it doesn't make an extra copy. This is not idiomatic Python, but it makes sense in C++

Something like:

n = 0
for s in results:
  if s not in ('.', '..'):  # better to use a tuple in mycpp, for efficient translation
    out.append(s)
    n += 1
return n     

This is necessary to keep it failing with the recent change to glob._py.
@andychu andychu merged commit fa2dae4 into soil-staging Dec 13, 2024
18 checks passed
@andychu
Copy link
Contributor

andychu commented Dec 13, 2024

Great, thank you!

Now I remember I didn't start a Zulip thread about how to implement globstar! I was wondering how it behaves, and the tests really helped, particularly knowing that it's different in bash and zsh

@leungbk leungbk deleted the leungbk-dotglob branch December 13, 2024 18:54
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.

2 participants