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.

Sign up for GitHub

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

A more minimal Ignition to Gazebo server rename effort #263

Merged
merged 2 commits into from
Aug 15, 2022
Merged

A more minimal Ignition to Gazebo server rename effort #263

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
20c7955
Attempting a more minimal Ignition to Gazebo server rename effort
Jun 13, 2022
352f42d
Merge branch 'ign-fuel-tools4' into minimal_ign_gz_server_update
nkoenig Aug 15, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions include/ignition/fuel_tools/ModelIdentifier.hh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,11 @@ namespace ignition
/// \return True if the ModelIdentifier names are equal.
public: bool operator==(const ModelIdentifier &_rhs) const;

/// \brief Inequality operator.
/// \param[in] _rhs ModelIdentifier to compare.
/// \return True if the ModelIdentifier names are not equal.
public: bool operator!=(const ModelIdentifier &_rhs) const;

/// \brief Returns just the last part of the model name.
/// \return Model name.
public: std::string Name() const;
Expand Down
6 changes: 6 additions & 0 deletions src/ClientConfig.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,12 @@ class ignition::fuel_tools::ClientConfigPrivate
homePath, ".ignition", "fuel");

this->servers.push_back(ServerConfig());

// Add in fuel.gazebosim.org as another default server config.
ServerConfig gzServerConfig;
gzServerConfig.SetUrl(common::URI("https://fuel.gazebosim.org"));
gzServerConfig.SetVersion("1.0");
this->servers.push_back(gzServerConfig);
Copy link
Contributor

Choose a reason for hiding this comment

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

Just for completeness, is there an advantage to hardcoding it here as opposed to there

gz-fuel-tools/conf/config.yaml

Lines 3 to 6 in b3c253c

servers:
-
name: osrf
url: https://fuel.ignitionrobotics.org

In theory, I think that hardcoding it here makes it difficult, if not impossible for users of other servers to disable gazebosim.org. Not sure if any users at all care about that, but it's a use case supported to an extent.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are a couple problems associated with the yaml config file.

  1. I don't think it's possible to use the yaml config file when running ign gazebo.
  2. I believe the config file is only used with ign fuel if the --config option is specified.
  3. If either fuel.ignitionrobotics.org or fuel.gazebosim.org are missing in the ClientConfig, then worlds/models that use both URIs will not load.

I don't see an easy way around this.

}

/// \brief Clear values.
Expand Down
14 changes: 9 additions & 5 deletions src/ClientConfig_TEST.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ std::string cachePath()
TEST(ClientConfig, InitiallyDefaultServers)
{
ClientConfig config;
EXPECT_EQ(1u, config.Servers().size());
EXPECT_EQ(2u, config.Servers().size());
nkoenig marked this conversation as resolved.
Show resolved Hide resolved
}

/////////////////////////////////////////////////
Expand All @@ -88,7 +88,7 @@ TEST(ClientConfig, ServersCanBeAdded)
srv.SetUrl(common::URI("http://asdf"));
config.AddServer(srv);

ASSERT_EQ(2u, config.Servers().size());
ASSERT_EQ(3u, config.Servers().size());
EXPECT_EQ(std::string("http://asdf"), config.Servers().back().Url().Str());
}

Expand All @@ -97,9 +97,11 @@ TEST(ClientConfig, ServersCanBeAdded)
TEST(ClientConfig, CustomDefaultConfiguration)
{
ClientConfig config;
ASSERT_EQ(1u, config.Servers().size());
ASSERT_EQ(2u, config.Servers().size());
EXPECT_EQ("https://fuel.ignitionrobotics.org",
config.Servers().front().Url().Str());
EXPECT_EQ("https://fuel.gazebosim.org",
config.Servers()[1].Url().Str());

std::string defaultCacheLocation = common::joinPaths(
homePath(), ".ignition", "fuel");
Expand Down Expand Up @@ -134,11 +136,13 @@ TEST(ClientConfig, CustomConfiguration)

EXPECT_TRUE(config.LoadConfig(testPath));

ASSERT_EQ(3u, config.Servers().size());
ASSERT_EQ(4u, config.Servers().size());
EXPECT_EQ("https://fuel.ignitionrobotics.org",
config.Servers().front().Url().Str());
EXPECT_EQ("https://api.ignitionfuel.org",
EXPECT_EQ("https://fuel.gazebosim.org",
config.Servers()[1].Url().Str());
EXPECT_EQ("https://api.ignitionfuel.org",
config.Servers()[2].Url().Str());
EXPECT_EQ("https://myserver",
config.Servers().back().Url().Str());

Expand Down
12 changes: 8 additions & 4 deletions src/FuelClient.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -572,12 +572,16 @@ Result FuelClient::DownloadModel(const ModelIdentifier &_id,
if(!res)
return res;

for (auto dep : dependencies)
for (ModelIdentifier dep : dependencies)
{
auto dep_res = this->DownloadModel(dep, _headers);
// Download dependency if not in the local cache
if (!this->dataPtr->cache->MatchingModel(dep))
{
auto dep_res = this->DownloadModel(dep, _headers);
nkoenig marked this conversation as resolved.
Show resolved Hide resolved

if(!dep_res)
return dep_res;
if(!dep_res)
return dep_res;
}
}

return res;
Expand Down
8 changes: 5 additions & 3 deletions src/FuelClient_TEST.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -538,12 +538,14 @@ TEST_F(FuelClientTest, DownloadModel)
EXPECT_EQ(ResultType::FETCH_ALREADY_EXISTS, res5.Type());
}

// Download model with a dependency specified within its `model.config`
// Download model with a dependency specified within its `model.config`.
// The dependency points to fuel.gazebosim.org.
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it valuable to also test the other way around, i.e. a ignitionrobotics URL that depends on a gazebosim URL?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The reverse also works. I've just added the ability to use both fuel.ignitionrobotics.org and 'fuel.gazebosim.org' at the same time. As long as both server configurations are present, then it'll work.

{
common::URI url{
"https://fuel.ignitionrobotics.org/1.0/JShep1/models/hatchback_red_2"};
"https://fuel.ignitionrobotics.org/1.0/openrobotics/models/hatchback red"
};
common::URI depUrl{
"https://fuel.ignitionrobotics.org/1.0/JShep1/models/hatchback_2"};
"https://fuel.gazebosim.org/1.0/openrobotics/models/hatchback"};

// Check it is not cached
std::string cachedPath;
Expand Down
3 changes: 0 additions & 3 deletions src/LocalCache.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,10 +99,7 @@ std::vector<Model> LocalCachePrivate::ModelsInServer(
{
std::vector<Model> models;
if (!common::isDirectory(_path))
{
ignwarn << "Server directory does not exist [" << _path << "]\n";
return models;
}

common::DirIter end;
common::DirIter ownIter(_path);
Expand Down
6 changes: 6 additions & 0 deletions src/ModelIdentifier.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,6 +135,12 @@ bool ModelIdentifier::operator==(const ModelIdentifier &_rhs) const
return this->UniqueName() == _rhs.UniqueName();
}

//////////////////////////////////////////////////
bool ModelIdentifier::operator!=(const ModelIdentifier &_rhs) const
{
return !(*this == _rhs);
}

//////////////////////////////////////////////////
ModelIdentifier::~ModelIdentifier()
{
Expand Down
2 changes: 2 additions & 0 deletions src/ModelIdentifier_TEST.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,10 +105,12 @@ TEST(ModelIdentifier, CopyConstructorDeepCopy)
EXPECT_EQ(2048u, id2.FileSize());
EXPECT_EQ(d1, id2.ModifyDate());
EXPECT_EQ(d2, id2.UploadDate());
EXPECT_EQ(id, id2);

id2.SetName("hello2");
EXPECT_EQ(std::string("hello"), id.Name());
EXPECT_EQ(std::string("hello2"), id2.Name());
EXPECT_NE(id, id2);
}

/////////////////////////////////////////////////
Expand Down