Skip to content

Dependency file parsing assumes one file per line #136

Open
@matthijskooijman

Description

@matthijskooijman
Collaborator

Apparently, the parsing of .d files assumes there is a single file on each line of the file. However, in some cases, presumably when filenames are short, there might be more than one in a single line. Consider this example from this report.

C:\Users\Graham\AppData\Local\Temp\builde33ce9ddee6346054afe0349d71c85f0.tmp\libraries\UTFT\UTFT.cpp.o: \
 F:\Arduino\libraries\UTFT\UTFT.cpp F:\Arduino\libraries\UTFT\UTFT.h \
 C:\Users\Graham\AppData\Local\Arduino15\packages\arduino\hardware\sam\1.6.7\cores\arduino/Arduino.h \
 (rest of the file removed)

The relevant source is here: https://github.com/arduino/arduino-builder/blob/master/src/arduino.cc/builder/builder_utils/utils.go#L222-L233

I guess the solution here is to do more proper parsing of the file, such as removing any escaped newlines, and then splitting the result into filenames (taking into account escaped spaces). I'm not entirely sure how reliable this can be done, since GNU make is known to be bad in handling spaces in filenames (though I think it works ok as long as no variables are involved).

Activity

ghlawrence2000

ghlawrence2000 commented on Apr 26, 2016

@ghlawrence2000

Ok, thanks Matthijs.

ghlawrence2000

ghlawrence2000 commented on May 4, 2016

@ghlawrence2000

Time passes by........... and nothing happens.........?

PaulStoffregen

PaulStoffregen commented on May 4, 2016

@PaulStoffregen
Sponsor

This worked properly when I implemented in Java.

But to be fair, I didn't handle spaces in pathnames properly until much later. So many little details to get right...

ghlawrence2000

ghlawrence2000 commented on May 4, 2016

@ghlawrence2000

Hi Paul, Is that something I can do?

cmaglie

cmaglie commented on May 4, 2016

@cmaglie
Member

IMHO the difference between the original java version and this one is in the loop here:

    for _, row := range rows {
        depStat, err := os.Stat(row)
        if err != nil && !os.IsNotExist(err) {
            return false, i18n.WrapError(err)
        }
        if os.IsNotExist(err) {
            return false, nil
        }
        if depStat.ModTime().After(objectFileStat.ModTime()) {
            return false, nil
        }
    }

IIRC the java version returned false if the file is not found, this one instead throws an error. Probably the java version will always trigger a full rebuild of the sketch in this case, but probably this is better and more conservative than throwing an error, I'll make a PR for this in a moment.

The long term solution is to properly parse the .d file, that may have more than one file per line, this could be tricky because the file are separated by space " " but a file may contains spaces escaped by backslash "\ ", so we could not simply split the line by spaces.

added a commit that references this issue on May 4, 2016
28f242e
cmaglie

cmaglie commented on May 4, 2016

@cmaglie
Member

Well, to be precise, there is already the condition if os.IsNotExist(err) { that should handle the "file not found" error, the problem is that in this case the error is different we have a:

GetFileAttributesEx F:\Arduino\libraries\UTFT\UTFT.cpp F:\Arduino\libraries\UTFT\UTFT.h: The filename, directory name, or volume label syntax is incorrect.

so in some way the parsed filename F:\Arduino\libraries\UTFT\UTFT.cpp F:\Arduino\libraries\UTFT\UTFT.h is able to trigger this subtle error instead of "file not found".

matthijskooijman

matthijskooijman commented on May 5, 2016

@matthijskooijman
CollaboratorAuthor

Wouldn't it be better to keep this issue open until it is really properly fixed? Also, I think that a proper fix might share some code with a fix for this comment: #131 (comment)

ghlawrence2000

ghlawrence2000 commented on May 5, 2016

@ghlawrence2000

Hi mathhijs,

The supplied arduino-builder.exe cures the problem, but as suspected, rebuilds all each time. Are you talking about a fix that doesn't rebuild if there is a problem? That would be better.

cmaglie

cmaglie commented on May 5, 2016

@cmaglie
Member

Wouldn't it be better to keep this issue open until it is really properly fixed

yes, github auto-closed the issue becuase of my comment on the PR.

reopened this on May 5, 2016

8 remaining items

Loading
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    topic: codeRelated to content of the project itselftype: enhancementProposed improvement

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

      Development

      No branches or pull requests

        Participants

        @matthijskooijman@cmaglie@PaulStoffregen@ghlawrence2000@per1234

        Issue actions

          Dependency file parsing assumes one file per line · Issue #136 · arduino/arduino-builder