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

rename .pdb file name as targetname. #162

Merged
merged 8 commits into from
Aug 4, 2015
Merged

Conversation

leeonix
Copy link
Contributor

@leeonix leeonix commented Jul 18, 2015

issue: #151

@starkos
Copy link
Member

starkos commented Jul 21, 2015

As I mentioned on #151, we'd ideally like to keep the default VS behavior if we can. It would be good to know how and when VS decides to rename that file on its own; we might be able to apply the same behavior.

But as far as this request, it should probably be $(OutDir)%(Filename).pdb, though I think there is a symbol that does all of that. $(OutputFile)?

Also, the tests need to pass.

@leeonix
Copy link
Contributor Author

leeonix commented Jul 22, 2015

yeah, your opinion is quite well. I modify from basename to VS macro name $(TargetName)

@starkos
Copy link
Member

starkos commented Jul 22, 2015

Great, that looks better. I'd like to take a quick look at how VS does things by default before I merge this. I will try to do it this weekend if I can!

@akaStiX
Copy link
Contributor

akaStiX commented Jul 28, 2015

I remember I had issues with Edit and Continue feature, if my pdb's were named after the project's name.
http://industriousone.com/topic/accept-my-bugfixes-please

@starkos
Copy link
Member

starkos commented Jul 29, 2015

had issues with Edit and Continue feature, if my pdb's were named after the project's name

Hmm, that's a good reason for more testing. Though not everyone wants their PDB in $(IntDir).

@leeonix: Maybe we should instead add a new API like pdbname() and pdbdir() so that we can leave the default behavior as is, but allow scripts to override it?

@TurkeyMan
Copy link
Contributor

Just a quick thought; I have vague recollection of some of the VS macros changing names between an older version (2008/2010-ish?) and the recent versions. I recall having some projects that were broken because the macro's didn't resolve... is there anyone who can confirm with confidence that this isn't one of the ones that changed name? Perhaps I'm imagining this?

@starkos
Copy link
Member

starkos commented Jul 30, 2015

have vague recollection of some of the VS macros changing names between an older version (2008/2010-ish?) and the recent versions…is there anyone who can confirm with confidence that this isn't one of the ones that changed name?

Currently, we don't output a at all and just let VS use its own defaults. So there is no variable being used that could have changed and broken. Unless I misunderstood your question?

@leeonix
Copy link
Contributor Author

leeonix commented Jul 31, 2015

@starkos sorry i'm late. move house recently, very tired.
Someone like me need .pdb file, the minimum requirement is .pdb name. directory isn't real needed. Becaure it's name write in PE file when linked over. I think if we need new API, pdbname() is enough.

@akaStiX what do you think?

@tvandijck
Copy link
Contributor

It used to be this:

    function m.programDataBaseFileName(cfg)
        if cfg.debugformat ~= "c7" and cfg.flags.Symbols then
            local filename = cfg.buildtarget.basename
            p.w('<ProgramDataBaseFileName>$(OutDir)%s.pdb</ProgramDataBaseFileName>', filename)
        end
    end

we changes it back to that... not sure why it was removed, but that is pretty much the behavior Leeonix is looking for I think.... and I reaallly don't want a pdbname API.....

@leeonix
Copy link
Contributor Author

leeonix commented Jul 31, 2015

@tvandijck First modification, I use cfg.buildtarget.basename too.
what do you think using $(TargetName)?

@tvandijck
Copy link
Contributor

Yeah, that should be possible indeed... probably even more generic...

anyway, I don't mind this as a local change here, it's an easy merge... and alternatively we can just override it in our module... that's kind of what the comment in that function suggest we should do anyway.

@TurkeyMan
Copy link
Contributor

@starkos This patch introduces use of $(OutDir)$(TargetName), which I'm not sure if either are among the VS macros that changed names some number of VS versions back.
I think it's fine, I couldn't google that either of those changed names.

@akaStiX
Copy link
Contributor

akaStiX commented Jul 31, 2015

It used to be this:

    function m.programDataBaseFileName(cfg)
        if cfg.debugformat ~= "c7" and cfg.flags.Symbols then
            local filename = cfg.buildtarget.basename
            p.w('<ProgramDataBaseFileName>$(OutDir)%s.pdb</ProgramDataBaseFileName>', filename)
        end
    end

we changes it back to that... not sure why it was removed

@tvandijck, I guess, it was my request, since this broke Edit & Continue in VS.
Check this comment #162 (comment)

@starkos
Copy link
Member

starkos commented Jul 31, 2015

@akaStiX: any chance you could cut-and-paste the code above back into that function and see if you can reproduce your issue? I tried quickly here and couldn't, but I may not have matched your setup.

Otherwise I'm fine with putting the code back in. Like I said above, I removed the markup only to match the default behavior of VS; if it is causing issues for people it should go back in (perhaps bit a comment to remind me why we need it so I don't take it out again in the future :p )

@akaStiX
Copy link
Contributor

akaStiX commented Jul 31, 2015

@starkos I am running VS2015 ATM and I cannot reproduce the issue I had there. But it was affecting VS2012 back in the days, when I've faced it.

@starkos
Copy link
Member

starkos commented Jul 31, 2015

@leeonix: can you update the request to match the code provided by @tvandijck?

if cfg.debugformat ~= "c7" and cfg.flags.Symbols then

I will put together a VS 2012 test and see if I can reproduce the problem and, if not (it may have been fixed by a service pack) we'll merge this and see if anyone yells. Sound good?

@leeonix
Copy link
Contributor Author

leeonix commented Aug 1, 2015

@starkos Yeah, I'll do it.

@tritao
Copy link
Contributor

tritao commented Aug 2, 2015

I'm all for merging this, the old behaviour of naming the PDB the same as the output target makes more sense than the VS default IMHO.

starkos added a commit that referenced this pull request Aug 4, 2015
rename .pdb file name as targetname.
@starkos starkos merged commit d8782c0 into premake:master Aug 4, 2015
@starkos
Copy link
Member

starkos commented Aug 4, 2015

Thanks!

@starkos
Copy link
Member

starkos commented Sep 4, 2015

Reposting here so more people will see it:

I've been asked to revisit this and to figure out why this change was necessary.

I manually generated a bunch of projects through the UI, for various versions of Visual Studio. The default Program Database File Name is, indeed, $(IntDir)vc$(PlatformToolsetVersion).pdb. However, on every build I tried this file is also copied to $(TargetDir)/$(TargetName).pdb, and this happens automatically, without the addition of a <ProgramDataBaseFileName> element.

Since it appears in my testing that we get the desired behavior without the need for this element, I'm inclined to roll back #162 and allow Visual Studio to use its default behavior again. Can you provide any more information on why you felt you needed to change it? Is minidump not able to use the PDB file that is already in the target directory?

starkos added a commit to starkos/premake-core that referenced this pull request Sep 9, 2015
As discussed in the conversion on the request, and on issue premake#151.
starkos added a commit that referenced this pull request Sep 10, 2015
TurkeyMan added a commit that referenced this pull request Nov 1, 2017
tvandijck pushed a commit to Blizzard/premake-core that referenced this pull request Nov 21, 2017
tvandijck added a commit that referenced this pull request Nov 29, 2017
Resolve the rule properties for gmake (#162)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants