Skip to content

Solve a problem of nmake build(use vs2017) #17

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

Closed
wants to merge 3 commits into from

Conversation

hxznhf
Copy link

@hxznhf hxznhf commented Dec 27, 2018

Solve a problem of nmake build(use vs2017)

@kjellahl
Copy link
Contributor

kjellahl commented Jan 2, 2019

There are a number of problems with this PR.

  • You don't explain which problem it solves.
  • One of the 3 commits has no commit message.
  • One of the commits adds a file with Chinese characters both in its name and its contents.
  • You modify the .pc.in in sigc++config.h.in files in such a way that your
    modifications are hard to follow. Are the .pc files used by MSVC? The are made for
    the pkg-config command in Linux systems.

Your PR needs rework.

@fanc999 Chun-wei, since this concerns NMake, can you comment? However, I would not
mind if you wait until the reporter has explained what's the problem.

@fanc999
Copy link
Collaborator

fanc999 commented Jan 3, 2019

Hi,

I am also having trouble trying to understand the issue that this attempts to solve...

Note that the files that were updated in this PR should have the proper entries in the release source tarball, so I don't think the changes are needed for this regard.

With blessings, and cheers!

@fanc999
Copy link
Collaborator

fanc999 commented Jan 3, 2019

Oh, also...

Is there a particular need for you to modify the .pc.in file, since Visual Studio builds do not generally use pkg-config files, unless when used with build systems like Meson.

@kjellahl kjellahl added the question Further information is requested label Jan 8, 2019
@murraycu
Copy link
Contributor

murraycu commented Jan 10, 2019

Note that the files that were updated in this PR should have the proper entries in the release source tarball

You might like to mention that in the new building section in the README.md. Thanks.

@kjellahl
Copy link
Contributor

@hxznhf Please explain which problem your PR solves. You can for instance include
error messages from VS2017. If it's not obvious from the error messages, you also
need to explain why you need to modify files that MSVS usually does not use.

@murraycu

You might like to mention that in the new building section in the README.md.

I suppose this comment is meant for Chun-wei Fan, but I don't understand what
you want mentioned. Shall he mention that all files that this PR modifies are,
as far as he knows, correct in the latest tarball?

IMO this PR shall be closed (not merged) unless the reporter gives more information
within a reasonable time. Do you want it to stay open until README.md has been
updated?

@kjellahl
Copy link
Contributor

kjellahl commented Mar 9, 2019

Closing this PR without merging. The reporter has not replied to requests for more information.
Feel free to reopen, if you can supply the requested information.

@kjellahl kjellahl closed this Mar 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants