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

Support for #define #1766

Closed
jnm2 opened this issue Aug 27, 2017 · 9 comments · Fixed by #1858
Closed

Support for #define #1766

jnm2 opened this issue Aug 27, 2017 · 9 comments · Fixed by #1858
Assignees
Labels
Milestone

Comments

@jnm2
Copy link
Contributor

jnm2 commented Aug 27, 2017

The Cake script

#define SOME_SYMBOL

Causes:

error CS1032: Cannot define/undefine preprocessor symbols after first token in file

(Version 0.21.1+Branch.main.Sha.9ff85bb53ea1890fabeb709d38ba79d3bf5cb16f)

Is there a technical reason this can't work or am I the first to try?

@bjorkstromm
Copy link
Member

@jnm2 there are technical reasons why it does't currently work. This is due to code generation done for aliases behind the scenes. Have a look at this gist showing the difference between plain .cake snd what's passed to Roslyn.

That said, there's no technical reason why we couldn't get it to work. But first, what is your use case?

@jnm2
Copy link
Contributor Author

jnm2 commented Aug 29, 2017

Cool! My use case is that I prefer #define SOME_SYMBOL to use with #if SYMBOL rather than const bool someSymbol = true; with if (someSymbol). Purely style.

Shouldn't be too hard to hoist #defines to the top then?

@jnm2
Copy link
Contributor Author

jnm2 commented Oct 3, 2017

I have a real use case now: avoiding warning CS0162: Unreachable code detected when I use a const bool instead of preprocessor.

Would you take a PR for this?

@devlead
Copy link
Member

devlead commented Oct 3, 2017

If you like to submit a PR we'll review it.

Tried this in csi.exe

#define DEBUG
#if (DEBUG)
Console.WriteLine("hello");
#endif

it works there, so should work in Cake too, but it needs to come first in the file so these defines would need to be moved to top of script like using statements much similar to what I propose in #1854 .

@jnm2
Copy link
Contributor Author

jnm2 commented Oct 3, 2017

much similar to what I propose

If it's fresh in your mind, would you do it? That would be perhaps more efficient.

@bjorkstromm
Copy link
Member

To spin this even further I’d say we should support defines via cmdline/config/environment (similar to DefineConstants in MSBuild). Also we should take #ifdef into account when preprocessing. This would open up possibility to conditionally load another file or conditionally install tool/addin.

@devlead
Copy link
Member

devlead commented Oct 3, 2017

If it's fresh in your mind, would you do it? That would be perhaps more efficient.
@jnm2 even if that were true, it still takes some effort to do it and time is limited 😎 But if no one takes it on I might take a stab at it when I can, have a few outstanding PRs awaiting @cake-build/cake-team review and could potentially look at this post that. But other things might also take precedence.

@mholo65
So tested plain scripting and csi.exe

#if (FOO)
#r ".\Newtonsoft.Json.dll"
#endif


Console.WriteLine(
    Newtonsoft.Json.JsonConvert.SerializeObject(
            new {
                Hello = "World"
            }
        )
    );

and it works regardless if i define FOO or not, so would seem as #r is processed at same scope as #if/#define
and gives error CS0103: The name 'Newtonsoft' does not exist in the current context if i remove the line.

I propose we first align with what Roslyn / csi.exe does as that's likely a fairly simple fix and later on look if we can / should improve it in any way.

I do like the DefineConstants as cmdline/config/environment idea though - essentially just a cake config for constants, makes a lot of sense.

devlead added a commit to devlead/cake that referenced this issue Oct 5, 2017
@devlead devlead self-assigned this Oct 5, 2017
@devlead devlead added the Feature label Oct 5, 2017
@devlead devlead added this to the v0.23.0 milestone Oct 5, 2017
@devlead
Copy link
Member

devlead commented Oct 5, 2017

Submitted PR #1858 as a proposed fixed for this issue.

@jnm2
Copy link
Contributor Author

jnm2 commented Oct 5, 2017

Thank you @devlead!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants