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

swift 3 broken / swift 4 update #3181

Closed
tanner0101 opened this Issue Jan 14, 2018 · 15 comments

Comments

Projects
None yet
6 participants
@tanner0101
Copy link
Contributor

tanner0101 commented Jan 14, 2018

It looks like the swift3 dependency is installed incorrectly.

Setup vapor: ~/FrameworkBenchmarks/frameworks/Swift/vapor
Setup vapor: databases is installed!
Setup vapor: swift-build: error: /home/techempower/FrameworkBenchmarks/frameworks/Swift/vapor/Package.swift:1:8: error: no such module 'PackageDescription'
Setup vapor: import PackageDescription
Setup vapor:        ^
Setup vapor: exit(1): /home/techempower/FrameworkBenchmarks/installs/swift/usr/bin/swiftc --driver-mode=swift -I /home/techempower/FrameworkBenchmarks/installs/swift/usr/lib/swift/pm -L /home/techempower/FrameworkBenchmarks/installs/swift/usr/lib/swift/pm -lPackageDescription /home/techempower/FrameworkBenchmarks/frameworks/Swift/vapor/Package.swift -fileno 8

http://tfb-logs.techempower.com/round-15/preview-3/vapor/out.txt

The -lPackageDescription lib should be installed and exported when Swift is installed correctly.

Additionally, we should update this to Swift 4 now.

I'd be more than happy to make a PR if someone could point me to the scripts that are responsible for configuring Swift.

@msmith-techempower

This comment has been minimized.

Copy link
Member

msmith-techempower commented Jan 15, 2018

You can generally find language/servers/etc at toolset/setup/linux as named shell scripts. Specifically, swift.sh is where you would need to update swift.

@zloster

This comment has been minimized.

Copy link
Contributor

zloster commented Jan 15, 2018

Also the scripts in the Vapor framework itself need some attention:

Each one of them is requesting PostgreSQL, MySQL and MongoDB to be installed/started. When you build and test against PostgreSQL there is no need to activate another databases and data stores.

@twof

This comment has been minimized.

Copy link

twof commented Jan 15, 2018

So a more appropriate setup-postgresql.sh would look like

#!/bin/bash

fw_depends swift3 ctls cmysql postgresql

swift build -Xswiftc -DNOJSON -c release

.build/release/vapor-tfb-mongodb --env=production &

yeah? @zloster

@msmith-techempower

This comment has been minimized.

Copy link
Member

msmith-techempower commented Jan 15, 2018

Why would the release be called vapir-tfb-mongodb if it depends on postgresql.

Also, what is cmysql?

@zloster

This comment has been minimized.

Copy link
Contributor

zloster commented Jan 15, 2018

@msmith-techempower

This comment has been minimized.

Copy link
Member

msmith-techempower commented Jan 15, 2018

IMO cmysql.sh should be called vapor.sh

Agreed

It's Vapor and something else?! https://github.com/TechEmpower/FrameworkBenchmarks/blob/master/toolset/setup/linux/systools/cmysql.sh#L9-L10

But it doesn't outwardly appear to use mongodb at all; what am I missing?

@zloster

This comment has been minimized.

Copy link
Contributor

zloster commented Jan 15, 2018

@msmith-techempower I was too slow to edit the comment. cmysql appears like some sort of bridge to the DB drivers. But I don't have any idea how swift/vapor connects to mongodb.

@msmith-techempower

This comment has been minimized.

Copy link
Member

msmith-techempower commented Jan 15, 2018

@zloster I think that the setup scripts are just wrong (potentially); there are three separate source folders for each database type (vapor-tfb-mongodb, vapor-tfb-mysql, vapor-tfb-postgresql), but each setup script relies on the default build process (not selecting one) which creates (I guess) vapor-tfb-mongodb by default.

I would expect each respective setup_...sh files to build their correct application server, and then execute the correct one rather than just the mongodb variant.

@msmith-techempower

This comment has been minimized.

Copy link
Member

msmith-techempower commented Jan 15, 2018

You're right and I'm dumb >_<

Well, that's good to know for sure.

I guess let's just clean up cmysql to be vapor?

@zloster

This comment has been minimized.

Copy link
Contributor

zloster commented Jan 15, 2018

@twof @msmith-techempower I'm also dumb >_<
In the script that should build the PostgreSQL variant you should declare postgresql as dependency and call postgresql build artifact. Not the mongodb one.
So the code should look like as:

#!/bin/bash

fw_depends swift3 ctls cmysql postgresql

swift build -Xswiftc -DNOJSON -c release

.build/release/vapor-tfb-postgresql --env=production &

With the agreement that fw_depends cmysql should be renamed to fw_depends vapor and the cmysql script renamed to vapor.

@tanner0101

This comment has been minimized.

Copy link
Contributor Author

tanner0101 commented Jan 15, 2018

As long as a Postgres server is available to be connected to over TCP, Vapor 3 should no longer require those C dependencies (for MySQL as well). We have pure-Swift drivers now.

All we should need to fix is the Swift PackageDescription issue as well as updating to Swift >= 4.0 since we depend on features in Swift 4.

@msmith-techempower I will look into that file you linked, thanks! :)

@michaelhixson

This comment has been minimized.

Copy link
Member

michaelhixson commented Feb 1, 2018

I think we're running to this problem: https://bugs.swift.org/browse/SR-5800

The build gets much farther if I change toolset/setup/linux/languages/swift3.sh to use the latest swift 4.1 snapshot.

fw_get -O https://swift.org/builds/development/ubuntu1404/swift-DEVELOPMENT-SNAPSHOT-2018-01-30-a/swift-DEVELOPMENT-SNAPSHOT-2018-01-30-a-ubuntu14.04.tar.gz
fw_untar swift-DEVELOPMENT-SNAPSHOT-2018-01-30-a-ubuntu14.04.tar.gz
mv swift-DEVELOPMENT-SNAPSHOT-2018-01-30-a-ubuntu14.04 swift

It eventually runs into an error compiling vapor. The version of vapor we're using is missing this fix: vapor/vapor@5a31fc8#diff-4a85e78227146848570c0c42d00d2f17

If I edit frameworks/Swift/vapor/Package.swift to use a more recent version of vapor...

    .Package(url: "https://github.com/vapor/vapor.git", "2.4.4"),

...then it passes all the TFB tests locally.

@msmith-techempower How do you feel about us using a snapshot version of Swift for the time being?

@msmith-techempower

This comment has been minimized.

Copy link
Member

msmith-techempower commented Feb 1, 2018

I'm fine with that.

michaelhixson added a commit to michaelhixson/FrameworkBenchmarks that referenced this issue Feb 1, 2018

Upgrade to swift 4, repair vapor build
This fixes issue TechEmpower#3181.

I think we were running into this issue in the stable vapor release:
https://bugs.swift.org/browse/SR-5800

...which the comments claim is really about this issue:
https://bugs.swift.org/browse/SR-5802

...which was fixed in the main swift source repository:
apple/swift-package-manager#1419

That fix happened pretty recently and doesn't seem to be included in the
latest stable releases (I don't see the PackageManager.swiftdoc file in
those releases), but it does seem to be fixed in the development snapshots
(I do see PackageManager.swiftdoc there).

With the development snapshot, the vapor build progresses much farther.
It eventually runs into an error compiling the vapor framework.  That is
an error in the outdated version of vapor we were using, and it was fixed
later:
vapor/vapor@5a31fc8#diff-4a85e78227146848570c0c42d00d2f17

When I specify 2.4.4 as our version of vapor (the most recent release),
the build is successful and all the tests pass.

I renamed "swift3.sh" to "swift.sh" instead of "swift4.sh" because we
prefer to keep the language version out of these filenames unless we
can't avoid having multiple versions of the same language in the toolset.

It looked like vapor's setup-*.sh files were declaring unnecessary
database dependencies, so I cleaned those up.

nbrady-techempower added a commit that referenced this issue Feb 3, 2018

Upgrade to swift 4.1 and mongodb 3.6, fix vapor tests (#3242)
* Upgrade to swift 4, repair vapor build

This fixes issue #3181.

I think we were running into this issue in the stable vapor release:
https://bugs.swift.org/browse/SR-5800

...which the comments claim is really about this issue:
https://bugs.swift.org/browse/SR-5802

...which was fixed in the main swift source repository:
apple/swift-package-manager#1419

That fix happened pretty recently and doesn't seem to be included in the
latest stable releases (I don't see the PackageManager.swiftdoc file in
those releases), but it does seem to be fixed in the development snapshots
(I do see PackageManager.swiftdoc there).

With the development snapshot, the vapor build progresses much farther.
It eventually runs into an error compiling the vapor framework.  That is
an error in the outdated version of vapor we were using, and it was fixed
later:
vapor/vapor@5a31fc8#diff-4a85e78227146848570c0c42d00d2f17

When I specify 2.4.4 as our version of vapor (the most recent release),
the build is successful and all the tests pass.

I renamed "swift3.sh" to "swift.sh" instead of "swift4.sh" because we
prefer to keep the language version out of these filenames unless we
can't avoid having multiple versions of the same language in the toolset.

It looked like vapor's setup-*.sh files were declaring unnecessary
database dependencies, so I cleaned those up.

* Upgrade mongdb from 3.2 to 3.6

The vapor tests rely on a feature that was added in mongodb 3.4, so we
might as well take this opportunity to upgrade mongodb to the latest
release.

* Disable vapor+mongodb multi-query and updates endpoints

They fail verification locally because they take too long to produce a
response.

* Remove vapor's Package.pins file, which is out of date and maybe unused?

It's showing the wrong version of Vapor, at least.  Also, it's not clear
to me if "Package.pins" is still used in this version of swift or if it
was replaced by "Package.resolved".

* Add index for world.id in mongodb

Some frameworks query for worlds by _id, and others by id.  It looks like
the ones that queried by id were being penalized heavily because we had
no index on that field.  In local testing, the RPS of nodejs-mongodb in
the db test went up by 8x (!) after I added the index.

Use createIndex instead of ensureIndex because, according to mongodb's
documentation, ensureIndex is:

  Deprecated since version 3.0.0: db.collection.ensureIndex() is now an
  alias for db.collection.createIndex().

Remove comment with a link to mongodb optimization because it doesn't
seem to say anything related to what we're doing in this file.

* Try to prevent restexpress from creating its own index in mongodb

The restexpress build on travis is failing with an exception like this:

  Exception in thread "main" com.mongodb.MongoException: Index with name: id_1 already exists with different options
    at com.mongodb.CommandResult.getException(CommandResult.java:100)
    at com.mongodb.CommandResult.throwOnError(CommandResult.java:134)
    at com.mongodb.DBTCPConnector._checkWriteError(DBTCPConnector.java:142)
    at com.mongodb.DBTCPConnector.say(DBTCPConnector.java:183)
    at com.mongodb.DBTCPConnector.say(DBTCPConnector.java:155)
    at com.mongodb.DBApiLayer$MyCollection.insert(DBApiLayer.java:270)
    at com.mongodb.DBApiLayer$MyCollection.createIndex(DBApiLayer.java:365)
    at com.mongodb.DBCollection.createIndex(DBCollection.java:484)
    at com.mongodb.DBCollection.ensureIndex(DBCollection.java:560)
    at com.github.jmkgreen.morphia.DatastoreImpl.ensureIndex(DatastoreImpl.java:431)
    at com.github.jmkgreen.morphia.DatastoreImpl.ensureIndexes(DatastoreImpl.java:536)
    at com.github.jmkgreen.morphia.DatastoreImpl.ensureIndexes(DatastoreImpl.java:495)
    at com.github.jmkgreen.morphia.DatastoreImpl.ensureIndexes(DatastoreImpl.java:584)
    at com.github.jmkgreen.morphia.DatastoreImpl.ensureIndexes(DatastoreImpl.java:573)
    at com.strategicgains.repoexpress.mongodb.MongodbRepository.initialize(MongodbRepository.java:106)
    at com.strategicgains.repoexpress.mongodb.MongodbRepository.<init>(MongodbRepository.java:67)
    at hello.controller.persistence.WorldsMongodbRepository.<init>(WorldsMongodbRepository.java:14)
    at hello.config.Configuration.initialize(Configuration.java:53)
    at hello.config.Configuration.fillValues(Configuration.java:45)
    at com.strategicgains.restexpress.util.Environment.load(Environment.java:61)
    at com.strategicgains.restexpress.util.Environment.from(Environment.java:50)
    at com.strategicgains.restexpress.util.Environment.fromDefault(Environment.java:40)
    at hello.Main.loadEnvironment(Main.java:49)
    at hello.Main.main(Main.java:18)

Individual frameworks should not be attempting to modify the indexes in
the database.  Hopefully that's all this @indexed annotation was doing.
@nbrady-techempower

This comment has been minimized.

Copy link
Member

nbrady-techempower commented Feb 5, 2018

This is solved for now, however as noted in #3242 the vapor tests are not completing queries/updates in a reasonable amount of time.

zloster added a commit to zloster/FrameworkBenchmarks that referenced this issue Feb 7, 2018

Upgrade to swift 4.1 and mongodb 3.6, fix vapor tests (TechEmpower#3242)
* Upgrade to swift 4, repair vapor build

This fixes issue TechEmpower#3181.

I think we were running into this issue in the stable vapor release:
https://bugs.swift.org/browse/SR-5800

...which the comments claim is really about this issue:
https://bugs.swift.org/browse/SR-5802

...which was fixed in the main swift source repository:
apple/swift-package-manager#1419

That fix happened pretty recently and doesn't seem to be included in the
latest stable releases (I don't see the PackageManager.swiftdoc file in
those releases), but it does seem to be fixed in the development snapshots
(I do see PackageManager.swiftdoc there).

With the development snapshot, the vapor build progresses much farther.
It eventually runs into an error compiling the vapor framework.  That is
an error in the outdated version of vapor we were using, and it was fixed
later:
vapor/vapor@5a31fc8#diff-4a85e78227146848570c0c42d00d2f17

When I specify 2.4.4 as our version of vapor (the most recent release),
the build is successful and all the tests pass.

I renamed "swift3.sh" to "swift.sh" instead of "swift4.sh" because we
prefer to keep the language version out of these filenames unless we
can't avoid having multiple versions of the same language in the toolset.

It looked like vapor's setup-*.sh files were declaring unnecessary
database dependencies, so I cleaned those up.

* Upgrade mongdb from 3.2 to 3.6

The vapor tests rely on a feature that was added in mongodb 3.4, so we
might as well take this opportunity to upgrade mongodb to the latest
release.

* Disable vapor+mongodb multi-query and updates endpoints

They fail verification locally because they take too long to produce a
response.

* Remove vapor's Package.pins file, which is out of date and maybe unused?

It's showing the wrong version of Vapor, at least.  Also, it's not clear
to me if "Package.pins" is still used in this version of swift or if it
was replaced by "Package.resolved".

* Add index for world.id in mongodb

Some frameworks query for worlds by _id, and others by id.  It looks like
the ones that queried by id were being penalized heavily because we had
no index on that field.  In local testing, the RPS of nodejs-mongodb in
the db test went up by 8x (!) after I added the index.

Use createIndex instead of ensureIndex because, according to mongodb's
documentation, ensureIndex is:

  Deprecated since version 3.0.0: db.collection.ensureIndex() is now an
  alias for db.collection.createIndex().

Remove comment with a link to mongodb optimization because it doesn't
seem to say anything related to what we're doing in this file.

* Try to prevent restexpress from creating its own index in mongodb

The restexpress build on travis is failing with an exception like this:

  Exception in thread "main" com.mongodb.MongoException: Index with name: id_1 already exists with different options
    at com.mongodb.CommandResult.getException(CommandResult.java:100)
    at com.mongodb.CommandResult.throwOnError(CommandResult.java:134)
    at com.mongodb.DBTCPConnector._checkWriteError(DBTCPConnector.java:142)
    at com.mongodb.DBTCPConnector.say(DBTCPConnector.java:183)
    at com.mongodb.DBTCPConnector.say(DBTCPConnector.java:155)
    at com.mongodb.DBApiLayer$MyCollection.insert(DBApiLayer.java:270)
    at com.mongodb.DBApiLayer$MyCollection.createIndex(DBApiLayer.java:365)
    at com.mongodb.DBCollection.createIndex(DBCollection.java:484)
    at com.mongodb.DBCollection.ensureIndex(DBCollection.java:560)
    at com.github.jmkgreen.morphia.DatastoreImpl.ensureIndex(DatastoreImpl.java:431)
    at com.github.jmkgreen.morphia.DatastoreImpl.ensureIndexes(DatastoreImpl.java:536)
    at com.github.jmkgreen.morphia.DatastoreImpl.ensureIndexes(DatastoreImpl.java:495)
    at com.github.jmkgreen.morphia.DatastoreImpl.ensureIndexes(DatastoreImpl.java:584)
    at com.github.jmkgreen.morphia.DatastoreImpl.ensureIndexes(DatastoreImpl.java:573)
    at com.strategicgains.repoexpress.mongodb.MongodbRepository.initialize(MongodbRepository.java:106)
    at com.strategicgains.repoexpress.mongodb.MongodbRepository.<init>(MongodbRepository.java:67)
    at hello.controller.persistence.WorldsMongodbRepository.<init>(WorldsMongodbRepository.java:14)
    at hello.config.Configuration.initialize(Configuration.java:53)
    at hello.config.Configuration.fillValues(Configuration.java:45)
    at com.strategicgains.restexpress.util.Environment.load(Environment.java:61)
    at com.strategicgains.restexpress.util.Environment.from(Environment.java:50)
    at com.strategicgains.restexpress.util.Environment.fromDefault(Environment.java:40)
    at hello.Main.loadEnvironment(Main.java:49)
    at hello.Main.main(Main.java:18)

Individual frameworks should not be attempting to modify the indexes in
the database.  Hopefully that's all this @indexed annotation was doing.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment