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

Makefile variables assigned with := are interpreted as rules (tasks) #146

Closed
reedhedges opened this issue Mar 10, 2021 · 9 comments
Closed
Labels

Comments

@reedhedges
Copy link

reedhedges commented Mar 10, 2021

It looks like variables assigned with := in Makefiles are interpreted as rules.
For example:

VAR:=foo

Is a variable assignment, but VAR appears as a rule (task). Happens with whitespace too:

VAR := foo

And therefore multiple assignments appearing in the Makefile trigger "is already registered" errors, e.g.

VAR := foo
VAR := bar

Here is a complete makefile that demonstrates the problem:

rule1: rule2
    @echo rule1

rule2:
    @echo rule2

VAR1=foo

VAR2:=foo

VAR3 := foo

# Uncomment this to trigger "already registered" error:
#ifdef FLAG
#VAR4:=foo
#VAR5 := foo
#else
#VAR4:=bar
#VAR5 := bar
#endif
@reedhedges
Copy link
Author

I've started looking into it, any suggestions? It looks like it's detecting rules by finding lines containing : but not starting with whitespace, a comment or $, and then skips it if it matches patterns for pattern rules etc. Would it make sense to check lines once against a regular expression that more closely matches what a make non-pattern/non-special rule should be? Or use a regular expression to find any rule, then use the current checks for special/pattern rules? Thanks.

@reedhedges
Copy link
Author

Another case:

VAR6 = "String with a : in it"

parses a rule named "a" for the a preceding the : in the string.

@reedhedges
Copy link
Author

I guess the other approach to Makefiles would be only displaying rules listed in .PHONY? (i.e. just find .PHONY: in the Makefile and use those names, not trying to parse the rest of the file?) But this would cause certain Makefiles to not be usable from task explorer I guess.

@spmeesseman spmeesseman added acknowledged more info needed Need more information from o.p. labels Mar 12, 2021
@spmeesseman
Copy link
Owner

hi... if u want to drop your makefile here, ill test against it and make some fixes when i get a chance one of these weekends. or youre always welcome to submit a pr too

@spmeesseman
Copy link
Owner

duh, nm, you already posted something that replicates. ill look at it when i have some free time

@reedhedges
Copy link
Author

reedhedges commented Apr 29, 2021

Hello, was going to work on this a bit but ran into some issues. I don't know Typescript at all. I know a bit of Javascript but not too much experience. How should I build the taskexplorer extension? Using npm to build I'm getting errors like Argument of type 'string | TreeItemLabel' is not assignable to parameter of type 'string' (in file.ts line 113) and Property 'localeCompare' does not exist on tye 'string | TreeItemLabel'. I have nodejs version 12.18.2. (I also noticed that it doesn't have String matchAll() either, which I think is a somewhat recent addition to Javascript, so is this version too old?) Thanks.

@reedhedges
Copy link
Author

I have [email protected].

@reedhedges
Copy link
Author

OK I just added some toString() calls and it fixed the 'string | TreeItemLabel' conversion errors.

Here is a change to the Make provider that uses a simpler approach, it just uses one regex to find each rule and returns that list: master...reedhedges:fix-makefile-rule-detection

Do you think that's a good way of doing it? It's a bit conservative about how rules are named, there is a chance it might not match some users' rules if they are particularly weird. It definitely skips pattern rules and rules with variables in them.

It does match explicit file names now (e.g. the literal "program.exe:" rule in the test makefile), which I think might be different behavior, but that is useful in some Makefiles. Could be easily changed. (Or an option?)

(Sorry that the diff is a mess, old code is still there just commented out.)

Let me know what you think.

@reedhedges
Copy link
Author

reedhedges commented Apr 30, 2021

Here's a better patch to look at, just the make.ts changes: reedhedges@301fe10

spmeesseman added a commit that referenced this issue Jun 6, 2021
# [2.3.0](v2.2.0...v2.3.0) (2021-06-06)

### Bug Fixes

* explorer occassionally hangs / stops responding [fixes [#143](#143)] ([a68166e](a68166e))
* if a task label exists that itself is equal to a 1st level task group name of another task (as per the separator char), the tree breaks. [fixes [#149](#149)] ([3716ca7](3716ca7))
* Makefile variables assigned with := are interpreted as rules. [fixes [#146](#146)] ([7b878d8](7b878d8))
* maven settings string is blank ([7e87b01](7e87b01))
* vscode tasks group separtors fixed by last check-in. [fixes [#147](#147)] ([f5d5b32](f5d5b32))

### Documentation

* **readme:** update featires by version section ([07d9f5e](07d9f5e))

### Features

* add 10+ new app-publisher commands, set up w/ group separator task names ([ce9c8f0](ce9c8f0))
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants