textileio / go-textile

[DEPRECATED] Textile is a set of tools and infrastructure for building composable apps and services on the IPFS network
https://textile.io
MIT License
357 stars 43 forks source link

gomodule: fix Go 1.13 compilation #915

Closed jsign closed 4 years ago

jsign commented 4 years ago

Fixes #914

Update go-ipfs to master (b15edf2) which solves many version-broken indirect dependencies.

The go-ipfs version update side consequences was a changed API regarding plugin loader. As far as I can see, the fix I've made makes sense but would like some second eyes that might have a better context.

The change in the API seems to consider what the code was doing manually before. The border case of repoPath being non-existant seems to be well handled in the LoadDirectory code:

// LoadDirectory loads a directory of plugins into the plugin loader.
func (loader *PluginLoader) LoadDirectory(pluginDir string) error {
    if err := loader.assertState(loaderLoading); err != nil {
        return err
    }
    newPls, err := loadDynamicPlugins(pluginDir)
    if err != nil {
        return err
    }

    for _, pl := range newPls {
        if err := loader.Load(pl); err != nil {
            return err
        }
    }
    return nil
}

func loadDynamicPlugins(pluginDir string) ([]plugin.Plugin, error) {
    _, err := os.Stat(pluginDir)
    if os.IsNotExist(err) {
        return nil, nil
    }
    if err != nil {
        return nil, err
    }

    return loadPluginsFunc(pluginDir)
}

where loadDynamicPlugins returns a (non error) nil/empty slice if the directory doesn't exist.

With this change make test run correctly for Go 1.13 and Go 1.12 (I tested that also).

PD: It would be good to use b15edf2 of go-ipfs dependency in other repos to keep that version consistent.

jsign commented 4 years ago

Oh, I think it should be also necessary to change the textile/builder docker image for the unit-test job in CI. With this current CI check it guarantees 1.12 compat, so maybe we can leave it this way for this PR, and maybe make another with only changing the textile/builder version in .circleci/config.yml

sanderpick commented 4 years ago

Agreed, we can build a new version of the builder. Nice work on this. Merge away!