Make sure we don't break compilation with past broken units
ClosedPublic

Authored by apol on Nov 7 2016, 12:05 AM.

Details

Summary

Show a warning together with the old broken behavior

Diff Detail

Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
apol updated this revision to Diff 7950.Nov 7 2016, 12:05 AM
apol retitled this revision from to Make sure we don't break compilation with past broken units.
apol updated this object.
apol edited the test plan for this revision. (Show Details)
apol added reviewers: Frameworks, dfaure.
dfaure accepted this revision.Nov 7 2016, 7:55 AM
dfaure edited edge metadata.

The error message is a bit confusing, it sounds like the right file is ${_tmp_FILE}.
So I would suggest this instead

message(WARNING "${_tmp_FILE}: Broken \"File\" field, make sure it's pointing at the right file")

Also, shouldn't the regexp be File=(.*)\n ? Basically, anything in the File field that is not *.kcfg should lead to "Broken File Field", right?
So in fact this is a bugfix (not "old broken behaviour") because it gives a better error message in case the file says File=foo : it will say "Broken File Field" instead of "Couldn't read the File field" (which would send people on the wrong track ... the field is here, it can be read, it just has the wrong contents).

This revision is now accepted and ready to land.Nov 7 2016, 7:55 AM
apol closed this revision.Nov 7 2016, 12:03 PM
apol added a comment.Nov 7 2016, 1:46 PM
In D3287#61135, @dfaure wrote:

The error message is a bit confusing, it sounds like the right file is ${_tmp_FILE}.
So I would suggest this instead

message(WARNING "${_tmp_FILE}: Broken \"File\" field, make sure it's pointing at the right file")

Okay.

Also, shouldn't the regexp be File=(.*)\n ? Basically, anything in the File field that is not *.kcfg should lead to "Broken File Field", right?

No, the thing is that we were matching (.*kcfg).* and this lets File=something.kcfgc work despite no something.kcfgc doesn't exist at all.

So in fact this is a bugfix (not "old broken behaviour") because it gives a better error message in case the file says File=foo : it will say "Broken File Field" instead of "Couldn't read the File field" (which would send people on the wrong track ... the field is here, it can be read, it just has the wrong contents).

It's broken old behaviour because assuming people are going to put the correct filename with things in the end and we'll match the right thing is wong.

dfaure added a comment.Nov 7 2016, 2:06 PM

I knew everything in your last reply already ;)
I'm not sure you understood my suggestion though.

If someone writes File=foo, your code will output

Couldn't read the \"File\" field

while it would be better to output

Broken \"File\" field, make sure it's pointing at a *.kcfg file

Of course it could then abort after that error, we only need the fallback to ${CMAKE_MATCH_1} for the compatibility thing we're talking about, when the filename is *.kcfgc.
So my comment isn't about the compat thing but about a better error message in general. Minor, though, so you did well pushing it.