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

Various improvements #211

Merged
merged 9 commits into from
Apr 10, 2013
Merged

Various improvements #211

merged 9 commits into from
Apr 10, 2013

Conversation

CyberShadow
Copy link
Contributor

Of note is the new vibe.core.args, which no longer needs to import every module that wants to parse command-line / configuration file options.

This allows building/testing Vibe programs/components without having
to specify the .lib files on the compiler command line every time.
The old setup required that, for every component requiring a server
(e.g. a DB driver), the user had ALL components installed and running
on their development system (thus, wasn't very scalable).
This is to allow debuggers to break on an uncaught exception.
This removes the dependency to HTTP code in vibe.core.args.

Related changes:
- Refactor out UID/GID stuff to its own module, which is activated by
  importing it (done in vibe.vibe)
- Fix: Vibe will not ignore --uid/--gid if it's not running as root,
  and will print error messages accordingly
- Settings in vibe.conf and the command line use the same names
- Added test
Notably, remove the vibe.http.server import.
The Win32 driver does NOT use I/O completion ports.
@s-ludwig
Copy link
Member

s-ludwig commented Apr 7, 2013

Some comments:

  • tests: This a part that was not really set in stone yet, as it depends heavily on the automated test system that we are currently (slowly) setting up. Depending on the final decisions there, it is possible that we'll need to change everything back to one project (e.g. because building many separate test projects takes up a lot more time that a single project with all tests). Since those tests are not really meant for the general user of the library, it shouldn't really matter too much that all dependencies need to be installed.
  • command line arguments: Interesting, didn't know about Runtime.args. One issue with the distributed system is that now there is no central place where all possible flags are listed, but maybe this is actually a nice opportunity to implement a dynamic help screen that is built up during static initialization from all getOption calls...
  • pragma(lib): DUB and VPM use "pkg-config" to find the correct linker flags/library names for a certain library to adapt for differences between systems/installations. Using pragma(lib) as a shortcut for the common case may be a nice addition, but it should not interfere with the existing build scripts/tools here. Does pragma(lib) produce an error if the specified library is not found? Does GDC produce an error or does it just ignore the pragma?

@CyberShadow
Copy link
Contributor Author

Since those tests are not really meant for the general user of the library, it shouldn't really matter too much that all dependencies need to be installed.

Well, I don't want to install Redis or MongoDB in order to run or write tests.

I suppose version blocks would be nicer. However, first I would tackle the performance problems head-on. The tests import vibe.vibe, for a start.

Does pragma(lib) produce an error if the specified library is not found?

Yes, with ld. Is it a matter of just library search paths, or varying file names too?

Does GDC produce an error or does it just ignore the pragma?

I don't know. I doubt it produces an error.

Phobos uses pragma(lib) for curl, and it hasn't caused any issues that I know of.

@s-ludwig
Copy link
Member

s-ludwig commented Apr 7, 2013

Well, I don't want to install Redis or MongoDB in order to run or write tests.

Sorry, I missed that the current app.d just stupidly runs all tests sequentially. The idea is that the test to run can be selected (e.g. by command line switch or using a system similar to OpenAutomate). So no need to install dependencies for unused tests.

Does pragma(lib) produce an error if the specified library is not found?

Yes, with ld. Is it a matter of just library search paths, or varying file names too?

Usually just paths, but I think I got also different names sometimes in the past (on FreeBSD), plus it can be that multiple library files are needed or just a single one. And then there is the problem with "--as-needed" on newer Ubuntu versions (and probably others) that requires libraries with interdependencies to be passed in the right order to the linker. But of course that may not be an issue for the specific libraries in question here - I'd like to be reasonably sure about that, though.

@s-ludwig s-ludwig merged commit 095531b into vibe-d:master Apr 10, 2013
s-ludwig added a commit that referenced this pull request Apr 10, 2013
@s-ludwig
Copy link
Member

I've now merged everything and wrapped the libevent and openssl pragma(lib)s in a version(VibePragmaLib) block to avoid issues until it's properly tested/working in all environments.

s-ludwig added a commit that referenced this pull request Apr 10, 2013
…move

privilege lowering code to vibe.core.core.

Privilege lowering was broken as it happened before any calls to listenTCP() or
similar functions. It is now executed explicitly using lowerPrivileges(). Also
processCommandLineArgs() is now fixed to at least perform in a reasonably
similar way to how it used to do so that compatibility is not silently broken.

See also #211.
@CyberShadow
Copy link
Contributor Author

Hi, thanks! In the future, please feel free to be specific on what needs to be changed for pull requests to be merged without fixups on your part, so I don't steal too much of your time.

version(VibePragmaLib) works for me, I'll just add it to my generic build command.

@s-ludwig
Copy link
Member

Yes, I didn't look close enough and didn't realize at first that the privilege lowering was broken - but it was quick to fix and most of the code is really just the new help screen functionality. So no problem.

The VibePragmaLib can hopefully be removed one or two dub/vibe releases ahead when those automatically define some kind of DubBuild or LibsExternallyLinked kind of version. The default could then be switched to use the pragma, which basically makes more sense after all.

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.

2 participants