fix: treat paths as absolute paths

[Aslemammad]

Resolves #131

This my first Rust PR ever, so I'd like to hear suggestions and what other paths need to be absolute in the source code, some hints may help!

[guybedford]

Glad to introduce you to Rust! Highly recommend learning it.

So in task.rs there are a bunch of strings which are actually paths, in particular the target and dep strings. For example the Job struct has targets and deps strings on it which are relative paths - https://github.com/guybedford/chomp/blob/main/src/task.rs#L99.

There is quite a lot of wiring going on there. In theory if the tests pass we should be good though.

One other weird thing is rust canonicalizes windows paths into extended path format, something like //?/.... if I recall correctly. that may invalidate some other assumptions about how paths are structured.

If you can get comfortable with debugging via println! and dbg! it should be possible to trace the logics and handle any cases that come up. Don't feel discouraged though it's not an easy job either, but thanks for giving it a shot!

[Aslemammad]

@guybedford Thank you so much for the opportunity, solving this would help me so much with learning rust!

[guybedford]

This is looking great, nice work. Does it handle windows path canonicalization? We should definitely have tests to land this feature.

Here are some cases that come to mind:

[[task]]
target = '../output.txt'
deps = ['../input.txt', './another.txt']

And ensuring that works.

Also worth checking globbing works:

[[task]]
deps = ['../*.txt']

And also interpolation:

[[task]]
target = '../#.output'
deps = ['./input/#.input']

And interpolation with globbing:

[[task]]
deps = ['../*.output']

[Aslemammad]

I'll work on these tests so we can deliver this properly, Thanks for the hints.

[Aslemammad]

I tried including those cases in the test/chompfile, you can check it out in the changes.

[Aslemammad]

@guybedford Fixed those suggestions. Thanks. I tried watch and serve and they work pretty well, let me know if you find any issues.

[guybedford]

Were files rerunning eg interpolated builds properly under watch? I would check that manually before merging.

For the correction - the point is to avoid any copy eg like using to owned. Rather you could do a filter first to filter to just those tasks with a name then do an unwrap and direct string equality check without copying.

On Fri, Sep 16, 2022 at 09:46 Mohammad Bagher Abiat < @.***> wrote:

@guybedford https://github.com/guybedford Fixed those suggestions. Thanks. I tried watch and serve and they work pretty well, let me know if you find any issues.

— Reply to this email directly, view it on GitHub https://github.com/guybedford/chomp/pull/140#issuecomment-1249576001, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAESFSSUIUVLXG65OT6MJM3V6SP6DANCNFSM6AAAAAAQACDMLE . You are receiving this because you were mentioned.Message ID: @.***>

[Aslemammad]

Feel free to check it out now! I saw the watcher, and they were absolute paths, still, you can check it out if you want so we get so closer to merging this.

[guybedford]

The extensions test failure looks like a valid bug in this PR to me:

thread 'main' panicked at 'index out of bounds: the len is 35 but the index is 35', src/task.rs:616:32

Can you take a look at that?

[Aslemammad]

I cannot reproduce it, is there anyway we let GitHub actions run without permission, so I can debug it here in gh actions without disrupting you for an access.

[guybedford]

@Aslemammad sure I've added you as a collaborator so you should have permissions now. Also check the commit hash of the chomp extensions repo being tested against to try locally.

[Aslemammad]

Thank you so much, Sure.

[Aslemammad]

Well, I'm running on the latest chomp-extensions repo, so it does not fail. trying here.

[Aslemammad]

New error, ncc shows that it does not support absolute paths, and we should change the config, any idea?

[guybedford]

@Aslemammad perhaps we should just make the $TARGET and $DEP environent variables still be relative paths with backtracking as necessary, rather than absolute for backwards compatibility?

[Aslemammad]

I got no problem with that, gonna try it, but what is backtracking exactly?

[Aslemammad]

It is working now.

[Aslemammad]

Sure, updated them. I may be less available these days, because the government is shutting down the internet and it makes it hard to keep up with coding, Sorry.

#MahsaAmini

[guybedford]

The core tests seem to be failing now unfortunately. Are you able to update and merge? I can get it landed otherwise.

[Aslemammad]

@guybedford Done.

[guybedford]

Thanks again for your hard work on this!