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

Fix a few build issues. #1

Merged
merged 7 commits into from
Jun 16, 2013
Merged

Fix a few build issues. #1

merged 7 commits into from
Jun 16, 2013

Conversation

jd28
Copy link
Member

@jd28 jd28 commented Jun 14, 2013

Here is a patch for a few outstanding issues. Some have been mention on the NWNX forum, some not.

best,
jmd

Changes:

  • .gitignore: Add lines for autoconf generated files.
  • CMakeLists.txt: Set cmake policy CMP0018 to OLD for
    compatibility with cmake 2.8.9 and higher. See:
    http://www.manpagez.com/man/1/cmakepolicies/
  • configure: Delete, doesn't need to be commited.
  • install.sh: Script was adding 'plugins/$plugins' to
    extraplugins string which when passed to configure
    would then also add 'plugins/' and would attempt to find
    'plugins/plugins/$plugin'.
  • plugins/optimizations: Rename Makefile -> Makefile.in. Not sure the intention, but
    cmake clobbers it.
  • plugins/spells: Add -Wno-error=unused-but-set-variable.

Edit: Added patch to fix the error in the nwnx2,ini concatenation part of the install.sh script.

@niv
Copy link
Contributor

niv commented Jun 15, 2013

  • AFAIR people would keep around "configure" because it's what they want to quickly set up a compile. This is a old point of discussion .. :) (I'm only maintaining the cmake part anyways)
  • plugins/optimizations Makefile was probably a oversight.
  • cmake policy might as well be NEW as long as it compiles?

The rest looks good, tyvm.

@jd28
Copy link
Member Author

jd28 commented Jun 15, 2013

I'm not very knowledgeable about cmake, honestly, so figured it would best to default to OLD in case other changes were necessary. The only differences that I could see is that NEW setting injected -fPIC into C_FLAGS & CXX_FLAGS, which had been explicitly removed before.

@niv
Copy link
Contributor

niv commented Jun 16, 2013

Can you please redo this PR with ./configure not being removed? It's still a point of contention for vman, since it's what people use to compile, and they don't neccessarily have automake installed.

@virusman
Copy link
Member

Also, could you split the changes into separate commits?

jmd added 7 commits June 16, 2013 04:25
Needed for compatibility with cmake 2.8.9 and higher.
See: http://www.manpagez.com/man/1/cmakepolicies/
Ignore the return type errors from the v2.8 api.
Ignore the no return value errors from v2.8 api.
- Script was adding 'plugins/$plugins' to
  extraplugins string which when passed to configure
  would then also add 'plugins/' and would attempt to find
  'plugins/plugins/$plugin'.

- Test for file existance doesn't support wildcards and so was
  throwing an unexpected command error when attempting to
  concatenate plugin specific nwnx2.ini files.
@jd28 jd28 closed this Jun 16, 2013
@jd28
Copy link
Member Author

jd28 commented Jun 16, 2013

Will do.

@virusman
Copy link
Member

You can rewrite the history of this branch (force push) and this PR will be updated automatically.

@virusman virusman reopened this Jun 16, 2013
@jd28
Copy link
Member Author

jd28 commented Jun 16, 2013

Oops, will remember in the future. The branch has been updated with the new commits now.

I add a couple other make file changes to suppress the -Wreturn-type errors being cause by the 2.8 api.

@niv
Copy link
Contributor

niv commented Jun 16, 2013

Looking good now, thanks for your work!

niv added a commit that referenced this pull request Jun 16, 2013
@niv niv merged commit 312a19d into NWNX:master Jun 16, 2013
@jd28 jd28 deleted the build-issues branch June 16, 2013 12:22
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.

3 participants