Skip to content

Regroup data: scripts and function updates#4229

Merged
lrineau merged 46 commits intoCGAL:masterfrom
gdamiand:CGAL_data-gdamiand
Sep 23, 2021
Merged

Regroup data: scripts and function updates#4229
lrineau merged 46 commits intoCGAL:masterfrom
gdamiand:CGAL_data-gdamiand

Conversation

@gdamiand
Copy link
Member

@gdamiand gdamiand commented Sep 18, 2019

Please use the following template to help us managing pull requests.

Summary of Changes

Start to regroup all data in a same global directory (for now only for LCC package).
Add a global function that computes the name of a data file using either a macro defined by cmake, or an environement variable.

Plan for Data

  • Move all data of examples in the directory Data/data/XXX (XXX being meshes, polylines, points_2, points_3)

Release Management

  • Affected package(s): LCC (for now)
  • Link to small feature here -- pre-approval date: 2021/01/18

TODOs

  • Try to remove .cmd files in examples
  • Write a script detecting duplicated files and moving them + updating examples/tests using them
  • write get_data_file_path() that will warn the user when the file does not exists + print CGAL_DATA_DIR

@sloriot
Copy link
Member

sloriot commented Oct 29, 2019

@gdamiand What about not moving all the files, but only general OFF/OBJ/STL/XXX ? Everything that is specific to a test or example can stay where it is. What do you think?

@gdamiand
Copy link
Member Author

I agree.

@MaelRL MaelRL changed the base branch from master to releases/CGAL-4.14-branch March 26, 2020 20:05
@MaelRL MaelRL changed the base branch from releases/CGAL-4.14-branch to master March 26, 2020 20:05
@MaelRL MaelRL added this to the Trash / Attic milestone May 11, 2020
@lrineau lrineau removed this from the Trash / Attic milestone Jul 9, 2020
@lrineau
Copy link
Member

lrineau commented Jul 9, 2020

@maxGimeno Once the CMake testsuite is done (issue #4826), I would like we take over this PR to resurrect it. But I think we can only do it correctly once the CMake testsuite is there: we will add a test or example that uses the get_data() function in the set of programs that are run automatically by the CMake testsuite.

@gdamiand
Copy link
Member Author

gdamiand commented Jul 9, 2020

Thanks @lrineau ; I agree to resurrect this PR.

Before we should discuss a little bit about the directories we want. I will create a small feature for that.

@gdamiand
Copy link
Member Author

gdamiand commented Jul 9, 2020

Cf this small feature to discuss about naming and directories
https://cgal.geometryfactory.com/CGAL/Members/wiki/Features/Small_Features/Regroup_data

@MaelRL MaelRL added Cleaning Not yet approved The feature or pull-request has not yet been approved. Small feature labels Oct 14, 2020
@MaelRL MaelRL added this to the 5.3-beta milestone Oct 14, 2020
@gdamiand
Copy link
Member Author

gdamiand commented Jan 7, 2021

@maxGimeno @lrineau cf. my updated proposal for data in the wiki.

@sloriot
Copy link
Member

sloriot commented Jan 7, 2021

@gdamiand I think it requires a second round as some decisions have already been taken from the discussion, Laurent even suggested a new name for the function.

@gdamiand
Copy link
Member Author

gdamiand commented Jan 7, 2021

@gdamiand I think it requires a second round as some decisions have already been taken from the discussion, Laurent even suggested a new name for the function.

Done.

@maxGimeno
Copy link
Contributor

There are conflicts

@gdamiand
Copy link
Member Author

gdamiand commented Jan 7, 2021

There are conflicts

I will solve them later; we need first to decide what to do on the wiki small feature page.

@sloriot
Copy link
Member

sloriot commented Jan 7, 2021

@gdamiand no work should be done to move files before #4255 is integrated. In particular this branch uses uppercase OFF, STL, PLY IIRC, so it might be good to respect that for the hierarchy. We can have a live meeting once the small feature is approved to discuss the details and who is doing what.

@gdamiand
Copy link
Member Author

gdamiand commented Jan 7, 2021

@gdamiand no work should be done to move files before #4255 is integrated. In particular this branch uses uppercase OFF, STL, PLY IIRC, so it might be good to respect that for the hierarchy. We can have a live meeting once the small feature is approved to discuss the details and who is doing what.

Ok @sloriot , thanks !

@sloriot sloriot added the pre-approved For pre-approved small features. After 15 days the feature will be accepted. label Jan 18, 2021
@sloriot
Copy link
Member

sloriot commented Jan 18, 2021

@gdamiand let me know when you want to have a meeting about this feature.

@gdamiand gdamiand changed the title Regroup all data Regroup data in examples Jan 19, 2021
@sloriot
Copy link
Member

sloriot commented Jul 21, 2021

I'll have a look.

Copy link
Member

@sloriot sloriot left a comment

Choose a reason for hiding this comment

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

After trying hard I could reproduce the error locally.
ERROR: Cannot run test script, please set the variable CGAL_DATA_DIR

I just know have to fix it (should not be hard).

@gdamiand
Copy link
Member Author

congratulation @sloriot !! (one day, we should modify/simplify our test suite...)

@sloriot
Copy link
Member

sloriot commented Jul 27, 2021

I could make the testsuite locally working

@gdamiand
Copy link
Member Author

I could make the testsuite locally working

Thanks @sloriot !!!

@sloriot
Copy link
Member

sloriot commented Sep 23, 2021

Successfully tested in CGAL-5.4-Ic-57

@lrineau lrineau added the rm only: ready for master For the release team only: that indicates that a PR is about to be merged in 'master' label Sep 23, 2021
@lrineau lrineau merged commit e5ab920 into CGAL:master Sep 23, 2021
@lrineau lrineau removed the rm only: ready for master For the release team only: that indicates that a PR is about to be merged in 'master' label Sep 23, 2021
@gdamiand
Copy link
Member Author

Successfully tested in CGAL-5.4-Ic-57

Congratulation !!

@afabri
Copy link
Member

afabri commented Sep 23, 2021

Is it documented what that means for developers? Does it mean something for data directories in example directories....

@sloriot
Copy link
Member

sloriot commented Sep 23, 2021

Please wait for the other PR that is doing the dirty part. This one was only about scripts and function. (#5427)

@lrineau lrineau deleted the CGAL_data-gdamiand branch September 23, 2021 14:33
@sloriot sloriot mentioned this pull request Oct 7, 2021
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants