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

some fixes #723

Closed
wants to merge 6 commits into from
Closed

some fixes #723

wants to merge 6 commits into from

Conversation

Trass3r
Copy link
Contributor

@Trass3r Trass3r commented Sep 23, 2014

collection of local patches

redstar pushed a commit that referenced this pull request Sep 27, 2014
fix Issue 12121 - atomicLoad!(MemoryOrder.acq) doesn't require load barrier
@Trass3r Trass3r force-pushed the master branch 3 times, most recently from a554e77 to 3899c69 Compare September 29, 2014 16:22
@Trass3r
Copy link
Contributor Author

Trass3r commented Sep 29, 2014

Ok added some more fixes esp. for Windows builds, please comment @klickverbot @redstar
In particular on the portable installation commit, not 100% sure.

Summary of lost commit comments: We should document useful llvm flags like -print-before-all, -print-after-all and -pass-remarks-analysis on the wiki etc.

@Trass3r
Copy link
Contributor Author

Trass3r commented Sep 29, 2014

Hmm now I'm (suddenly?!) hitting the "can't find curl.lib" linker problem when building phobos-test-runner.

"-I@INCLUDE_INSTALL_DIR@/ldc",
"-I@INCLUDE_INSTALL_DIR@",
"-L-L@CMAKE_INSTALL_LIBDIR@", @MULTILIB_ADDITIONAL_INSTALL_PATH@
"-I%%ldcbinarypath%%/../include/d/ldc",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs to actually work with INCLUDE_INSTALL_DIR, as some distro packages use it. IIRC there are a couple of CMake functions for converting paths, though.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm I could modify the variables to be %%ldcbinarypath%%/../... but then install doesn't work anymore. Maybe a separate conf file and a cmake switch for portable installation would be the cleanest solution.

@Trass3r
Copy link
Contributor Author

Trass3r commented Oct 1, 2014

Changed the portable installation approach. Improvement suggestions welcome.

@dnadlinger
Copy link
Member

I merged everything but the portable installation stuff into master manually. Two issues I still see with the latter:

  • The term "portable installation" isn't exactly clear, especially in the Linux world. Maybe we can come up with a better name for the flag, but at least the description should mention that it enables relative paths. Even "relocatable" would be better, but it might be confusing w.r.t. relocatable executables.
  • You should probably make CMake error out if a custom INCLUDE_INSTALL_DIR is set in conjunction with your option to avoid producing a broken installation.

@Trass3r
Copy link
Contributor Author

Trass3r commented Oct 2, 2014

Thanks! Good idea to split it.
There is definitely room for improvement regarding the portable Installation. It's just a draft so far.

@dnadlinger
Copy link
Member

I presume you are familiar enough with Git to know how to remove the already-merged commits by rebasing? If you need help, just let me know.

@dnadlinger
Copy link
Member

@redstar, @AlexeyProkhin (and @Dicebot as one as our packages): Any creative ideas on how to handle this?

@mihails-strasuns
Copy link
Contributor

What exactly seems to be a problem? I currently do this to build ldc:

    cmake \
    -DCMAKE_INSTALL_PREFIX=/usr \
    -DINCLUDE_INSTALL_DIR=/usr/include/dlang/ldc \
    ..

..and looks like it will still work after the changes as portability is switched off my default.

@dnadlinger
Copy link
Member

@Dicebot: We can just go with Trass3r's change as is (maybe with an extra check that INCLUDE_INSTALL_DIR isn't set in addition to PORTABLE_INSTALL). I was just wondering whether we might be missing a better way to handle this.

@Trass3r
Copy link
Contributor Author

Trass3r commented Oct 6, 2014

Yeah feel free to throw in ideas :)
Is it possible that redstar doesn't receive notifications? Pinging him hasn't worked for me lately. Or commenting on his commit.

@redstar
Copy link
Member

redstar commented Oct 6, 2014

@Trass3r Sorry for not answering. I am a bit busy right now and do not have a good internet connection, too.

@mihails-strasuns
Copy link
Contributor

I don't have any strong opinion on topic as it is not something I would usually use.

@mihails-strasuns
Copy link
Contributor

Though I think removing any kind of hard-coded paths/prefixes in the binary (if there are any) and moving those to configuration is a good thing in general - then any sort of user-local installation can be done by simply copying relevant binaries.

@redstar
Copy link
Member

redstar commented Apr 3, 2015

I picked the assembly annotator: see #881.

@Trass3r
Copy link
Contributor Author

Trass3r commented Apr 21, 2015

Ok but please set the author correctly in the future for better tracking.

@dnadlinger
Copy link
Member

Same thing applies still; please consider reopening isolated parts (if they still apply) as individual pull requests.

@dnadlinger dnadlinger closed this Aug 28, 2015
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.

4 participants