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

lock1 fails if provided string contains folders #68

Closed
unode opened this Issue May 5, 2018 · 8 comments

Comments

Projects
None yet
2 participants
@unode
Copy link
Collaborator

unode commented May 5, 2018

samples = [
   "projectA/sampleA1",
   "projectB/sampleB1"
]
lock1(samples)

errors with:

[Sat 05-05-2018 17:07:20] Line 13: Interpreting [assignment]: lock1(Lookup 'samples' as NGList NGLString; __hash="d4531bdab467a3f1feb67e5ae87de081")
[Sat 05-05-2018 17:07:20] Line 13: Interpreting [executing module function: 'lock1']: NGOList [NGOString "projectA/sampleA1",NGOString "projectB/sampleB1"]
[Sat 05-05-2018 17:07:20] Line 13: Looking for a lock in ngless-locks/d4531bda. Total number of elements is 2 (not locked: 2; not finished: 2).
Exiting after internal error. If you can reproduce this issue, please run your script with the --trace flag and report a bug at http://github.com/luispedro/ngless/issues
ngless-locks/d4531bda/projectA/sampleA1.lock: lock: does not exist (No such file or directory)
@unode

This comment has been minimized.

Copy link
Collaborator Author

unode commented May 5, 2018

A simple workaround could be to replace any / or \ (windows) in the string by _.

@unode unode closed this in cd02257 May 17, 2018

luispedro added a commit that referenced this issue Jun 5, 2018

RLS Release v0.8.1
Bugfix release. Trigger for release was the updated conduit-algorithms
as it makes a huge difference:

    Version 0.8.1 2018-06-05 by luispedro
        * Update to LTS-11.12 (for faster conduit-algorithms used in collect())
        * Add fallback for character encoding on systems with bad locale support
        * Fixed lock1 when used with paths (#68)
        * Fixed expansion of searchdir with external modules (#56)

luispedro added a commit that referenced this issue Jun 5, 2018

RLS Release v0.8.1
Bugfix release. Trigger for release was the updated conduit-algorithms
as it makes a huge difference:

    Version 0.8.1 2018-06-05 by luispedro
        * Update to LTS-11.12 (for faster conduit-algorithms used in collect())
        * Add fallback for character encoding on systems with bad locale support
        * Fixed lock1 when used with paths (#68)
        * Fixed expansion of searchdir with external modules (#56)
@unode

This comment has been minimized.

Copy link
Collaborator Author

unode commented Sep 3, 2018

@luispedro Realized one use-case of this code that got broken with the above fix.

file_with_full_paths.txt

/path/to/file1
/another/path/to/file2
samples = readlines('file_with_full_paths.txt')
sample = lock1(samples)

sample now contains _path_to_file1 which follows the fix above makes the variable sample useless for users.
Do you see any problem in returning /path/to/file1 from lock1() regardless of the name used for the lock?

@luispedro

This comment has been minimized.

Copy link
Collaborator

luispedro commented Sep 3, 2018

Yeah, that's how it should work.

@luispedro luispedro reopened this Sep 3, 2018

@unode unode closed this in ec948ae Sep 4, 2018

@unode

This comment has been minimized.

Copy link
Collaborator Author

unode commented Sep 4, 2018

Fix in in place. Only missing a test-case to avoid future regressions.
Not sure how to go about testing this thought.

@luispedro

This comment has been minimized.

Copy link
Collaborator

luispedro commented Sep 4, 2018

Thanks! This should be mentioned in the ChangeLog and What's New page for 1.0

Test could be:

ngless "0.9"
print(lock1(['testing/slashes\\ and spaces']))
@unode

This comment has been minimized.

Copy link
Collaborator Author

unode commented Sep 4, 2018

That last was already https://github.com/ngless-toolkit/ngless/blob/master/tests/parallel_folder_lock/lock.ngl minus the print().
Now it tests this too.

@luispedro

This comment has been minimized.

Copy link
Collaborator

luispedro commented Sep 4, 2018

The idea of adding print is that you can test whether the result is correct by having a output.stdout.txt file.

@unode

This comment has been minimized.

Copy link
Collaborator Author

unode commented Sep 4, 2018

Yup: 2eff033 - What I was missing is that the tests run with --quiet.

luispedro added a commit that referenced this issue Nov 12, 2018

BLD Release 0.10
Many small fixes rather than any large new features.

Full ChangeLog:

    * Fix to lock1's return value when used with paths (#68 - reopen)
    * Support _F/_R suffixes for forward/reverse in load_mocat_sample
    * samtools_sort() now accepts by={name} to sort by read name
    * Fixed bug where header was printed even when STDOUT was used
    * Fixed bug where writing interleaved FastQ to STDOUT did not work as
    expected
    * Indices created by bwa and minimap2 are now versioned
    * arg1 in external modules is no longer always treated as a path
    * Added expand_searchpath to external modules API (closes #56)
    * Fixed bug where detection of Fastq encoding was not performed on the second pair
    * Fix saving fastq sets with --subsample (issue #85)
    * Add __extra_megahit_args to assemble() (issue #86)
    * Better error message when user mis-specifies the ngless version string
    (issue #84)
    * Support NO_COLOR environment variable (issue #83)
    * Garbage collection for temporary files (issue #79)
    * Rename --search-dir to --search-path for consistency with other API
    * Fix corner case with select() producing incorrect CIGAR strings (#92)
    * Always check output file writability (#91)
    * Make paired() accept encoding argument
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.