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

Increment Syntex Version to 0.30.0 #241

Closed
indiv0 opened this Issue Mar 22, 2016 · 14 comments

Comments

Projects
None yet
4 participants
@indiv0

indiv0 commented Mar 22, 2016

Would it be possible to increment the syntex version to 0.30.0 so that I can use Diesel with serde_codegen?

mcasper added a commit to mcasper/diesel that referenced this issue Mar 22, 2016

@sgrif

This comment has been minimized.

Member

sgrif commented Mar 23, 2016

Sorry, I've been away on travel. I'll make sure this gets dealt with as quickly as possible.

mcasper added a commit to mcasper/diesel that referenced this issue Mar 23, 2016

mcasper added a commit to mcasper/diesel that referenced this issue Mar 23, 2016

@sgrif

This comment has been minimized.

Member

sgrif commented Mar 23, 2016

Side note, that I don't have a better place to document. I'm thinking we should start reaching out to Serde and other prominent crates to loosen their syntex versions when possible (even if it means writing code differently to work on other versions). The shared dependency on Syntex is effectively forcing us (and others like dotenv, which hasn't actually had a code change associated with a syntex bump in ages) to churn immediately when any part of libsyntax changes, even if we aren't affected by it (e.g. our nightly tests still pass).

@mcasper mcasper closed this in #242 Mar 23, 2016

@indiv0

This comment has been minimized.

indiv0 commented Mar 23, 2016

@sgrif the problem as I see it is that diesel_codegen and other projects don't automatically use the latest version of syntex available. This is especially problematic because (as you describe), most times the update is not a breaking change.

A local solution to this problem (i.e., on your's and dotenv's end) would be to set the version requirements to >= 0.28.0. This would allow cargo to automatically bump to the latest version.
This would fix the behaviour currently caused by using the caret (^) operator, which for pre-1.0.0 versions limits the project to >= 0.28.0 < 0.29.0, so we only get patch-based changes.
This is, however, problematic because packages which use this method will automatically pull in breaking changes to syntex, which will break their local builds.

The actual, long-term solution appears to be for syntex to version their builds more correctly according to semver.
If these version bumps are indeed non-breaking changes, then the syntex maintainers should be using patch bumps instead of minor bumps while they are pre-1.0.0.

If syntex can be considered stable now, then the solution is slightly different but still similar: set the version to 1.0.0 and use the minor version for non-breaking changes.

I may be looking at this incorrectly, but for example, https://github.com/serde-rs/syntex/pull/38/files does not appear to introduce a breaking change to the syntex API but rather exposes a new method in the API, so it should've been a patch bump (as syntex is still pre-1.0.0).

I realize that semver says that pre-1.0.0 we shouldn't expect any consistency from APIs, but clearly this is resulting in an issue which needs to be worked around.

@erickt, @sgrif any thoughts or comments?

@indiv0

This comment has been minimized.

indiv0 commented Mar 23, 2016

Also it looks like @mcasper closed this issue (thank you btw!) just as I posted the comment, so yeah this is probably best discussed somewhere in a dedicated issue.

@mcasper

This comment has been minimized.

Collaborator

mcasper commented Mar 23, 2016

D: Sorry! Silly Github auto closing issues....

@sgrif

This comment has been minimized.

Member

sgrif commented Mar 23, 2016

I disagree, we do need to explicitly bump the version with each release. These changes are breaking code for someone, just not us as we're not relying on it. The problem is that there's urgency to do so with each release, since as soon as serde makes a hard version bump (and not a range), every other project is going to get a ton of issues opened. Sometimes this will be unavoidable (we actually had to make code changes going from 0.27.0 to 0.28.0), but I think it can be less than it is now.

@sgrif

This comment has been minimized.

Member

sgrif commented Mar 23, 2016

I also need to finish my work on making it more pleasant to use diesel on stable without syntex, but that still doesn't solve the problem unless we want to actually remove it as an option

@sgrif

This comment has been minimized.

Member

sgrif commented Mar 23, 2016

I've published v0.5.4 of diesel_codegen with the updated dependencies

@erickt

This comment has been minimized.

erickt commented Mar 23, 2016

@sgrif: any chance aster could help mitigate breaking changes?

@indiv0

This comment has been minimized.

indiv0 commented Mar 23, 2016

@sgrif Well since the result of syntex introducing a breaking change is forcing the update on everyone (not just the projects the change actually affects), then semver isn't able to do its job of allowing libraries which don't explicitly need it to stay on earlier releases.

The alternative workaround is then allowing libraries to use different versions of syntex independently.

I'm curious if individual packages re-exporting syntex could be a solution.
Would something like this be a plausible option for it?

#[cfg(feature = "with-syntex")]
mod inner {
    extern crate diesel_codegen;
    extern crate serde_codegen;

    use std::env;
    use std::path::Path;

    pub fn main() {
        let out_dir = env::var_os("OUT_DIR").unwrap();

        for &(src, dst) in &[
            ("src/main.in.rs", "main.rs"),
            ("src/api/v1/policy.in.rs", "policy.rs"),
            ("src/api/v1/tracks.in.rs", "tracks.rs"),
        ] {
            let src = Path::new(src);
            let dst = Path::new(&out_dir).join(dst);

            generate_diesel(src, dst);
            generate_serde(dst, dst);
        }
    }

    pub fn generate_diesel(src: Path, dst: Path) {
        let mut registry = diesel_codegen::syntex::Registry::new();
        diesel_codegen::register(&mut registry);
        registry.expand("", &src, &dst).unwrap();
    }

    pub fn generate_serde(src: Path, dst: Path) {
        let mut registry = serde_codegen::syntex::Registry::new();
        serde_codegen::register(&mut registry);
        registry.expand("", &src, &dst).unwrap();
    }
}

#[cfg(feature = "nightly")]
mod inner {
    pub fn main() {}
}

fn main() {
    inner::main();
}
@sgrif

This comment has been minimized.

Member

sgrif commented Mar 23, 2016

@erickt No, we recently dropped aster and quasi as dependencies, as they made the issue worse. The more transitive dependencies on syntex, the worse it is. We no longer have anything transitive, but everybody effectively has it as a transitive dependency through serde.

@erickt

This comment has been minimized.

erickt commented Mar 23, 2016

@sgrif: Ugh I'm sorry to hear that. Is this because you're trying to support nightly plugins at the same time as stable rust with syntex?

@sgrif

This comment has been minimized.

Member

sgrif commented Mar 23, 2016

Right. That's basically the use case for syntex. ;)

@indiv0

This comment has been minimized.

indiv0 commented Mar 24, 2016

@sgrif @erickt so is re-exporting syntex crates not a viable option?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment